Skip to content

[baremetal] Change to unicast for keepalived#1504

Closed
bcrochet wants to merge 2 commits intoopenshift:masterfrom
bcrochet:keepalived-unicast
Closed

[baremetal] Change to unicast for keepalived#1504
bcrochet wants to merge 2 commits intoopenshift:masterfrom
bcrochet:keepalived-unicast

Conversation

@bcrochet
Copy link
Copy Markdown
Member

@bcrochet bcrochet commented Feb 25, 2020

This implements a unicast solution for keepalived. Multicast is often not available
so unicast is a more common method.

WIP

- What I did

- How to verify it

- Description for the changelog

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 25, 2020
yboaron
yboaron reviewed Feb 26, 2020
View reviewed changes
Comment thread manifests/baremetal/keepalived.yaml Outdated
Copy link
Copy Markdown
Contributor

@yboaron yboaron Feb 26, 2020

Choose a reason for hiding this comment

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

The resources in manifest directory run in bootstrap node, right?
Do we need the keepalived-monitor in bootstrap?

Copy link
Copy Markdown
Member Author

@bcrochet bcrochet Feb 26, 2020

Choose a reason for hiding this comment

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

That is correct. And yes we do. We are writing the configuration dynamically.

Copy link
Copy Markdown
Contributor

@yboaron yboaron Feb 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

OK, got it - 10x

yboaron
yboaron reviewed Feb 26, 2020
View reviewed changes
Comment thread templates/master/00-master/baremetal/files/baremetal-keepalived-keepalived.yaml Outdated
Copy link
Copy Markdown
Contributor

@yboaron yboaron Feb 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

How do we handle the case where router pod run in worker node?

Copy link
Copy Markdown
Member Author

@bcrochet bcrochet Feb 26, 2020

Choose a reason for hiding this comment

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

Are you talking about the ingress VIP?

Copy link
Copy Markdown
Contributor

@yboaron yboaron Feb 27, 2020

Choose a reason for hiding this comment

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

Yep

Copy link
Copy Markdown
Member Author

@bcrochet bcrochet Mar 11, 2020

Choose a reason for hiding this comment

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

Ok. Addressed.

yboaron
yboaron reviewed Feb 27, 2020
View reviewed changes
Comment thread manifests/baremetal/keepalived.yaml Outdated
Copy link
Copy Markdown
Contributor

@yboaron yboaron Feb 27, 2020

Choose a reason for hiding this comment

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

I guess it should be: cpu: 100m && memory: 200Mi , right?

Copy link
Copy Markdown
Member Author

@bcrochet bcrochet Mar 11, 2020

Choose a reason for hiding this comment

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

Fixed.

yboaron
yboaron reviewed Feb 27, 2020
View reviewed changes
Comment thread manifests/baremetal/keepalived.yaml Outdated
Copy link
Copy Markdown
Contributor

@yboaron yboaron Feb 27, 2020

Choose a reason for hiding this comment

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

Why do we need to specify 'cluster-config' parameter for the bootstrap case?

Copy link
Copy Markdown
Member Author

@bcrochet bcrochet Mar 5, 2020

Choose a reason for hiding this comment

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

Mainly so I don't have to do a special case. It didn't work otherwise.

@openshift-ci-robot
Copy link
Copy Markdown
Contributor

openshift-ci-robot commented Feb 27, 2020

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bcrochet
To complete the pull request process, please assign runcom
You can assign the PR to them by writing /assign @runcom in a comment when ready.

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

Details 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

@bcrochet bcrochet force-pushed the keepalived-unicast branch from 0a2280b to e60669d Compare March 4, 2020 19:46
@bcrochet bcrochet changed the title [baremetal] WIP: Change to unicast for keepalived [baremetal] Change to unicast for keepalived Mar 5, 2020
@bcrochet bcrochet force-pushed the keepalived-unicast branch 4 times, most recently from cbc91b8 to 2509307 Compare March 9, 2020 20:01
@bcrochet
Change to unicast for keepalived
219ab0f 
This implements a unicast solution for keepalived. Multicast is often not available
so unicast is a more common method.
@bcrochet bcrochet force-pushed the keepalived-unicast branch 4 times, most recently from 03b3b87 to b37f1c8 Compare March 16, 2020 16:03
@bcrochet
Make unicast enablement optional
9e4bbe8 
This makes the unicast selected an environment variable used by
baremetal-runtimecfg.
@bcrochet bcrochet force-pushed the keepalived-unicast branch from b37f1c8 to 9e4bbe8 Compare March 17, 2020 19:58
@kikisdeliveryservice
Copy link
Copy Markdown
Contributor

kikisdeliveryservice commented Jun 23, 2020

@bcrochet this does not seem to be in active development? Do you need this PR open?

@kikisdeliveryservice
Copy link
Copy Markdown
Contributor

kikisdeliveryservice commented Jun 23, 2020

/skip e2e-ovn-step-registry

@kikisdeliveryservice
Copy link
Copy Markdown
Contributor

kikisdeliveryservice commented Jun 23, 2020

/skip

@openshift-ci-robot
Copy link
Copy Markdown
Contributor

openshift-ci-robot commented Jul 21, 2020

@bcrochet: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-aws 9e4bbe8 link /test e2e-aws
ci/prow/e2e-gcp-upgrade 9e4bbe8 link /test e2e-gcp-upgrade
ci/prow/e2e-ovn-step-registry 9e4bbe8 link /test e2e-ovn-step-registry
ci/prow/e2e-aws-proxy 9e4bbe8 link /test e2e-aws-proxy

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

@bcrochet
Copy link
Copy Markdown
Member Author

bcrochet commented Jul 22, 2020

Superseded by #1768

@bcrochet bcrochet closed this Jul 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@yboaron yboaron yboaron left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bcrochet @openshift-ci-robot @kikisdeliveryservice @yboaron