Skip to content

Conversation

@Rozzii
Copy link
Member

@Rozzii Rozzii commented Mar 27, 2025

This PR:

  • Adds read only file system support for the keepalived container
  • Extend the README with keepalived related information

These changes were needed because:

  • The previous implementation was not compatible with running the container
    with a read only root file system.

Related issue: metal3-io/ironic-image#613

P.S needs to be rebased after #27 is merged
/hold

@metal3-io-bot metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 27, 2025
@Rozzii
Copy link
Member Author

Rozzii commented Mar 27, 2025

/hold

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 27, 2025
@Rozzii Rozzii changed the title ✨ Keepalived read-only file-system support WIP ✨ Keepalived read-only file-system support Mar 27, 2025
@metal3-io-bot metal3-io-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 27, 2025
@metal3-io-bot metal3-io-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 28, 2025
@Rozzii
Copy link
Member Author

Rozzii commented Mar 28, 2025

/hold cancel

@metal3-io-bot metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 28, 2025
@Rozzii Rozzii changed the title WIP ✨ Keepalived read-only file-system support ✨ Keepalived read-only file-system support Mar 28, 2025
@metal3-io-bot metal3-io-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 28, 2025
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two readability nits, otherwise lgtm.

@Rozzii Rozzii requested a review from tuminoid March 28, 2025 14:47
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 28, 2025
Copy link
Member

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering about the "extra" vars, otherwise this looks good to me

This commit:
 - Adds read-only fie system support for the keepalived container
 - Extend the README with keepalived related information

These changes were needed because:
 - The previous implementation was not compatible with running the container
   with a read only root file system.

Signed-off-by: Adam Rozman <[email protected]>
@Rozzii Rozzii force-pushed the keepalive_ro_fs branch from 107c3c6 to b1cdbed Compare June 16, 2025 11:23
@metal3-io-bot metal3-io-bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 16, 2025
@Rozzii Rozzii requested review from lentzi90 and tuminoid June 16, 2025 11:24
@tuminoid
Copy link
Member

/test ?

@metal3-io-bot
Copy link
Collaborator

@tuminoid: The following commands are available to trigger required jobs:

/test markdownlint
/test shellcheck

Use /test all to run all jobs.

In response to this:

/test ?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

I created metal3-io/project-infra#1039 to remind us to add some e2e tests for this repo.

I've manually checked it does at least compile and run when caps are added. There shouldn't be any need for writing into filesystem.

Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 16, 2025
Copy link
Member

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@metal3-io-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lentzi90, tuminoid

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 16, 2025
@metal3-io-bot metal3-io-bot merged commit a98b876 into metal3-io:main Jun 16, 2025
6 checks passed
@metal3-io-bot metal3-io-bot deleted the keepalive_ro_fs branch June 16, 2025 13:12
@Rozzii
Copy link
Member Author

Rozzii commented Jun 16, 2025

/approve

I created metal3-io/project-infra#1039 to remind us to add some e2e tests for this repo.

I've manually checked it does at least compile and run when caps are added. There shouldn't be any need for writing into filesystem.

I was actually creating this:
I think e2e is overkill here, instead I would like to do this: #39

KEEPALIVED_CONF="${KEEPALIVED_CONF_DIR}/keepalived.conf"
KEEPALIVED_DATA_DIR="${CUSTOM_DATA_DIR}/keepalived"

mkdir -p "${KEEPALIVED_CONF_DIR}" "${KEEPALIVED_DATA_DIR}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is broken without root

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants