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

contrib/kind: adapt clustermesh related make targets to recent changes #24693

Merged
merged 3 commits into from Apr 12, 2023
Merged

contrib/kind: adapt clustermesh related make targets to recent changes #24693

merged 3 commits into from Apr 12, 2023

Conversation

giorio94
Copy link
Member

@giorio94 giorio94 commented Apr 3, 2023 •
edited

This PR fixes two issues concerning the clustermesh related make targets introduced by recent modifications:

  • make kind-clustermesh: do not fail if the kind-cilium docker network already exists
  • make kind-install-cilium-clustermesh: initialize the LOCAL_CLUSTERMESH_IMAGE variable

Additionally, it extends the kind-down.sh script and makes the kind-clustermesh-down target use it, ensuring that the kind-cilium network gets deleted also in that case.

More details are provided in the respective commit messages.

contrib/kind: adapt clustermesh related make targets to recent changes

@giorio94 giorio94 added the release-note/misc This PR makes changes that have no direct user impact. label Apr 3, 2023
@giorio94 giorio94 requested review from lmb, squeed and marseel April 3, 2023 09:43
@giorio94 giorio94 requested review from a team as code owners April 3, 2023 09:43
bimmlerd
bimmlerd requested changes Apr 3, 2023
View reviewed changes
Copy link
Member

@bimmlerd bimmlerd left a comment

Choose a reason for hiding this comment

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

I'd prefer changing kind-down instead of duplicating it's logic in the Makefile

Makefile Outdated Show resolved Hide resolved
@giorio94 giorio94 force-pushed the mio/make-kind-clustermesh-fix branch from cd7b486 to c3cb8eb Compare April 3, 2023 10:12
@giorio94 giorio94 requested a review from bimmlerd April 3, 2023 10:13
marseel
marseel approved these changes Apr 3, 2023
View reviewed changes
Copy link
Contributor

@marseel marseel left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

squeed
squeed approved these changes Apr 3, 2023
View reviewed changes
Copy link
Contributor

@squeed squeed left a comment

Choose a reason for hiding this comment

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

Sorry for the churn, thanks for cleaning up my mess :p

lmb
lmb reviewed Apr 3, 2023
View reviewed changes
Makefile
@@ -459,7 +458,7 @@ kind-clustermesh-images: kind-clustermesh-ready kind-build-clustermesh-apiserver
$(QUIET)kind load docker-image $(LOCAL_OPERATOR_IMAGE) --name clustermesh1
$(QUIET)kind load docker-image $(LOCAL_OPERATOR_IMAGE) --name clustermesh2

.PHONY: kind-install-cilium-clustermesh
Copy link
Contributor

Choose a reason for hiding this comment

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

So call KIND_ENV ends up doing the same as .PHONY?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it adds .PHONY among the others: https://github.com/cilium/cilium/blob/c3cb8eba315b8786a166e0c34bae78f7fd202bf4/Makefile#L444-L450

@lazedo
Copy link

lazedo commented Apr 3, 2023

maybe need to address the xdp parameter to not run it twice ? i would prefer if clustermesh used two distinct networks instead (now possible since the docker-cilium addition)

@giorio94
Copy link
Member Author

giorio94 commented Apr 4, 2023

maybe need to address the xdp parameter to not run it twice ?

Right now we are not setting --xdp in the clustermesh configuration, hence it does not create problems. Still, I think it would be cleared just not to specify --xdp in one of the invocations, rather than attempting to detect whether it had already been configured previously.

i would prefer if clustermesh used two distinct networks instead (now possible since the docker-cilium addition)

Do you mean having the two clustermesh clusters in different docker networks? Could you please elaborate about why you feel that would be better?

bimmlerd
bimmlerd approved these changes Apr 4, 2023
View reviewed changes
Copy link
Member

@bimmlerd bimmlerd left a comment

Choose a reason for hiding this comment

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

thanks!

@giorio94
Copy link
Member Author

/ci-datapath

@giorio94
contrib/kind: do not fail if the kind-cilium network already exists
0bbd532 
4310b5e modified the kind.sh contrib script introducing the
creation of a dedicated kind-cilium docker network to be used by kind.
Yet, this broke the `make kind-clustermesh` command, as the second
execution of the script fails given that the network already exists.
Hence, let's first check if the network already exists, and create it
only if not already present.

Fixes: 4310b5e ("contrib/kind: enable XDP_TX from pod veth")
Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94
contrib/kind: cleanup kind-cilium network on kind-clustermesh-down
761f080 
This commit parametrizes the `kind-down.sh` script, to tear down all
kind clusters specified as parameters (defaulting to kind if none are
specified), and changes the `kind-clustermesh-down` target to use it.
Hence, uniforming the cleanup logic to reduce possible discrepancies.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94
contrib/kind: fix uninitialized variable in install-clustermesh target
9b19cb4 
2975c75 dropped the usage of a local docker registry and, among
others, changed the `kind-install-cilium-clustermesh` target to use a
parametrized name for the clustermesh-apiserver image. Yet, that
variable was not initialized, causing the target to fail. This commit
fixes it, calling the initialization logic.

Fixes: 2975c75 ("contrib/kind: no longer create local docker registry")
Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
@giorio94 giorio94 force-pushed the mio/make-kind-clustermesh-fix branch from c3cb8eb to 9b19cb4 Compare April 11, 2023 12:11
@giorio94
Copy link
Member Author

Force pushed to rebase onto master

@giorio94
Copy link
Member Author

/ci-datapath

@giorio94
Copy link
Member Author

The Cilium datapath workflow seems to be currently broken (#24809). Given that the kind.sh script is used also by that workflow, I'll re-run it once the fix gets merged to ensure no regressions are introduced by this PR.

@giorio94 giorio94 mentioned this pull request Apr 11, 2023
Closed
@giorio94
Copy link
Member Author

ConformanceK8sKind hit unrelated flake: #24622
ConformanceIngress hit unrelated flake: #24349

@giorio94
Copy link
Member Author

/ci-datapath

@giorio94
Copy link
Member Author

Reviews are in, and ci-datapath (which uses kind-sh to build the clusters) passed. The other tests would not increase coverage as not using it. Marking as ready to merge.

@giorio94 giorio94 added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 11, 2023
@dylandreimerink dylandreimerink merged commit f59ae7e into cilium:master Apr 12, 2023
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lmb lmb lmb approved these changes

@squeed squeed squeed approved these changes

@borkmann borkmann borkmann approved these changes

@marseel marseel marseel approved these changes

@bimmlerd bimmlerd bimmlerd approved these changes

Assignees
No one assigned
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@giorio94 @lazedo @lmb @squeed @borkmann @marseel @bimmlerd @dylandreimerink