Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

move keepalived here from BMO repo #27

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

move keepalived here from BMO repo #27

wants to merge 1 commit into from

Conversation

Rozzii
Copy link
Member

@Rozzii Rozzii commented Jan 31, 2025 •
edited
Loading

This PR:

  • Moves the project used to build the Metal3 keepalived container from
    the BMO repository to this repository
  • Adds support for customizable config file location for the keepalived
    container
  • Adds container building github workflow for keepalived
  • Adds keepalived release workflow
  • Adds release document for the whole of the utility-images repository

These changes were needed for two related reasons.

  • The community has decided that there is no reason to keep the keepalived
    files in BMO and they much better fit for the utility-images repository.
  • There is ongoing work to turn the ironic pod compatible with the K8s pod
    security option that enforces the use of read only mode for the container
    file system and the current containers deployed as part of the Ironic pod
    such as keepalived are not compatible without modification.

Note: release workflows and documentation is a new concept for this
repository and a bit differ from other Metal3 repos because different
projects within the repo get released independently.

Related issues:

@metal3-io-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from rozzii. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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 size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 31, 2025
@Rozzii Rozzii force-pushed the keepalived_move branch 2 times, most recently from b3b5153 to 3b0d749 Compare January 31, 2025 12:17
tuminoid
tuminoid reviewed Jan 31, 2025
View reviewed changes
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.

Need to bring in the image building workflow as well, as well as parts of release workflow, and parts of release documentation too.

@metal3-io-bot metal3-io-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 31, 2025
@Rozzii Rozzii force-pushed the keepalived_move branch 3 times, most recently from 307ec43 to fd9e677 Compare January 31, 2025 13:32
@Rozzii
Copy link
Member Author

Rozzii commented Jan 31, 2025 •
edited
Loading

What is left to discuss with the community what release tags to use in the future for keepalived, as right now I don't see any reason to follow BMO releases/ release tags for keepalived.

The other PR that removes keepalived from BMO is also in progress.

I would also suggest doing a release of keepalived after this PR has been merged, as part of the release we would test whether everything works as expected and I would suggest choosing a release tag that is detached from BMO.

@Rozzii
move keepalived here from BMO repo
eba8b63 
This commit:
 - Moves the project used to build the Metal3 keepalived container from
   the BMO repository to this repository
 - Adds support for customizable config file location for the keepalived
   container
 - Adds container building github workflow for keepalived
 - Adds keepalived release workflow
 - Adds release document for the whole of the utility-images repository

These changes were needed for two related reasons.
 - The community has decided that there is no reason to keep the keepalived
   files in BMO and they much better fit for the utility-images repository.
 - There is ongoing work to turn the ironic pod compatible with the K8s pod
   security option that enforces the use of read only mode for the container
   file system and the current containers deployed as part of the Ironic pod
   such as keepalived are not compatible without modification.

Note: release workflows and documentation is a new concept for this
repository and a bit differ from other Metal3 repos because different
projects within the repo get released independently.

Signed-off-by: Adam Rozman <adam.rozman@est.tech>
@tuminoid
Copy link
Member

What is left to discuss with the community what release tags to use in the future for keepalived, as right now I don't see any reason to follow BMO releases/ release tags for keepalived.

No, we should release keepalived when there is relevant changes.

The other PR that removes keepalived from BMO is also in progress.

👍

I would also suggest doing a release of keepalived after this PR has been merged, as part of the release we would test whether everything works as expected and I would suggest choosing a release tag that is detached from BMO.

Absolutely. As we did with ironic-client, it is also good time to give it some attention, bump anything there is to bump etc, then release.

tuminoid
tuminoid reviewed Feb 2, 2025
View reviewed changes
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.

/hold

The release process needs to be agreed and then all the automation/docs adapted to match.

.github/workflows/keepalived-release.yaml
@@ -0,0 +1,138 @@
# This code is borrowed from https://github.com/kubernetes-sigs/cluster-api/blob/main/.github/workflows/release.yaml
Copy link
Member

Choose a reason for hiding this comment

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

I doubt we will use this automation for the keepalived with release branches etc.

.github/workflows/keepalived-release.yaml
fi
- name: Create Release Tag
run: |
git config user.name "${GITHUB_ACTOR}"
Copy link
Member

Choose a reason for hiding this comment

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

These are also all incorrect, etc.

README.md
provide fix IP address for Ironic in such a manner that even after pivoting
operations the IP of Ironic stays persistent.

[Keeplaived documentation](https://www.keepalived.org/manpage.html)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[Keeplaived documentation](https://www.keepalived.org/manpage.html)
[Keepalived documentation](https://www.keepalived.org/manpage.html)

RELEASING.md
tick pre-release box.
- GitHub jobs `build_keepalived` build release images with the
release tag, and push them to Quay. Make sure the release tags are visible in
Quay tags pages:
Copy link
Member

Choose a reason for hiding this comment

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

This need to match single image wording from IPAM etc.

RELEASING.md

Some post-release actions are needed if new minor or major branch was created.

### Branch protection rules
Copy link
Member

Choose a reason for hiding this comment

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

No release branches.

RELEASING.md
least once in the PR targeting the branch in question. Branch protection rules
require user to have `admin` permissions in the repository.

### Update README.md and build badges
Copy link
Member

Choose a reason for hiding this comment

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

There are no build badges.

Which also reminds me that probably need to have a periodic jobs for these, as we have not even pinned down the main ubuntu image.

@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 Feb 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tuminoid tuminoid tuminoid left review comments

@dtantsur dtantsur Awaiting requested review from dtantsur

@kashifest kashifest Awaiting requested review from kashifest

Assignees
No one assigned
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Metal3 - Roadmap
Status: Ironic-image WIP
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Rozzii @metal3-io-bot @tuminoid