Skip to content

Conversation

@mikemckiernan
Copy link

@mikemckiernan mikemckiernan commented Aug 11, 2021

MetalLB and IP failover (keepalived) are
incompatible. Technical info for this
commit originated in NE-572.


Preview the Removing IP failover topic.

@netlify
Copy link

netlify bot commented Aug 11, 2021

✔️ Deploy Preview for osdocs ready!

🔨 Explore the source changes: f2a00f7

🔍 Inspect the deploy log: https://app.netlify.com/sites/osdocs/deploys/61252a632c0ebf00089d6adc

😎 Browse the preview: https://deploy-preview-35436--osdocs.netlify.app

@openshift-ci openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 11, 2021
@mikemckiernan mikemckiernan added this to the Future Release milestone Aug 11, 2021
@mikemckiernan
Copy link
Author

PTAL and let me know what I need to change!

/assign @msherif1234
/assign @asood-rh

@mikemckiernan
Copy link
Author

@bmcelvee, can you recommend an SME from network edge to peek?

As we exchanged briefly in Slack, I figured it made the most sense to put this new procedure on the same page as everything else for IP failover.

@mikemckiernan mikemckiernan requested a review from bmcelvee August 11, 2021 17:40
@bmcelvee
Copy link
Contributor

@rfredette would you mind reviewing this PR, please? Thanks!

@mikemckiernan
Copy link
Author

@mikemckiernan Do you really want to call out IP table rules?

The page already has mention of an IP tables rule. Unless someone feels very strongly, my preference is to be as clear as possible that the job runs to reverse that rule. The rule is mentioned in OPENSHIFT_HA_IPTABLES_CHAIN. That said, I need to reformat and use iptables rule to match the existing text.

Copy link

@rfredette rfredette left a comment

Choose a reason for hiding this comment

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

lgtm; I had one nit-pick, but even if it's not addressed I think this looks good.

Comment on lines 71 to 65

Choose a reason for hiding this comment

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

Is it necessary to manually remove the security context constraints? I'm not super knowledgeable in this area, but I'd think sccs would be cleaned up when deleting the service account in the next step.

Copy link
Author

Choose a reason for hiding this comment

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

@rfredette, I think you are correct, or at least the key is that the doc specifies spec.serviceAccountName: ipfailover and so if the service account is deleted, then customers would need to play by some other rules than what is documented.

FWIW:

$ oc adm policy scc-review -z ipfailover -f ipfailover.yaml 
RESOURCE                           SERVICE ACCOUNT   ALLOWED BY   
Deployment/ipfailover-keepalived   ipfailover        privileged   


$ oc delete sa ipfailover
serviceaccount "ipfailover" deleted


$ oc adm policy scc-review -f ipfailover.yaml              
error: unable to compute Pod Security Policy Review for "ipfailover-keepalived": unable to retrieve ServiceAccount ipfailover: serviceaccount "ipfailover" not found

$ oc adm policy scc-review -z ipfailover -f ipfailover.yaml
error: unable to compute Pod Security Policy Review for "ipfailover-keepalived": unable to retrieve ServiceAccount ipfailover: serviceaccount "ipfailover" not found

Thank you.

Choose a reason for hiding this comment

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

Add namespace

Copy link
Contributor

@adellape adellape left a comment

Choose a reason for hiding this comment

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

Just the one comment, but otherwise LGTM.

Copy link
Contributor

Choose a reason for hiding this comment

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

Use colon before substeps (throughout).

MetalLB and IP failover (keepalived) are
incompatible.  Technical info for this
commit originated in NE-572.

* Feedback from Ryan F.
* Move step to locate config maps to before
  the step that deletes the pods.
* Use `iptables` to match existing formatting.

* Feedback from Mohamed and Arti.

* Review from Alex. End instructional text that introduces
  substeps with a colon unless another sentence intervenes.
@mikemckiernan mikemckiernan added the peer-review-done Signifies that the peer review team has reviewed this PR label Aug 24, 2021
@mikemckiernan mikemckiernan merged commit 20566ac into openshift:main Aug 24, 2021
@mikemckiernan
Copy link
Author

/cherrypick enterprise-4.9

@openshift-cherrypick-robot

@mikemckiernan: new pull request created: #35769

Details

In response to this:

/cherrypick enterprise-4.9

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/test-infra repository.

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

Labels

branch/enterprise-4.9 peer-review-done Signifies that the peer review team has reviewed this PR size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants