Skip to content

bgpv2: Introduce NoMatchingNode condition to CiliumBGPClusterConfig #35517

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
Merged
merged 2 commits into from
Oct 25, 2024
Merged

bgpv2: Introduce NoMatchingNode condition to CiliumBGPClusterConfig #35517

merged 2 commits into from
Oct 25, 2024

Conversation

YutaroHayakawa
Copy link
Member

@YutaroHayakawa YutaroHayakawa commented Oct 24, 2024 •
edited
Loading

Introduce a new condition to report the error that the given node selector selects nothing. This is the first condition we introduce in the CiliumBGPClusterConfig. Therefore, this PR also introduces an update for the CRD and an internal infra to set conditions.

apiVersion: cilium.io/v2alpha1
kind: CiliumBGPClusterConfig
metadata:
  name: cilium-bgp
spec:
  bgpInstances:
  - localASN: 65000
    name: instance0
  nodeSelector:
    matchLabels:
      select: nothing
status:
  conditions:
  - lastTransitionTime: "2024-10-24T07:34:05Z"
    message: spec.nodeSelector selects nothing
    observedGeneration: 3
    reason: NoMatchingNode
    status: "True"
    type: cilium.io/NoMatchingNode

bgpv2: Introduce NoMatchingNode condition to CiliumBGPClusterConfig

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 24, 2024
@YutaroHayakawa YutaroHayakawa added the release-note/ci This PR makes changes to the CI. label Oct 24, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 24, 2024
@YutaroHayakawa YutaroHayakawa added release-note/misc This PR makes changes that have no direct user impact. and removed release-note/ci This PR makes changes to the CI. labels Oct 24, 2024
rastislavs
rastislavs reviewed Oct 24, 2024
View reviewed changes
@YutaroHayakawa YutaroHayakawa force-pushed the yutaro/bgp-cluster-config-condition branch from 4960d02 to 5862378 Compare October 24, 2024 11:20
@YutaroHayakawa
bgpv2: Define and generate ClusterConfig condition
c72b046 
Introduce a status and conditions field in the CiliumBGPClusterConfig
which will be filled by the Cilium Operator for error reporting.

Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
@YutaroHayakawa YutaroHayakawa force-pushed the yutaro/bgp-cluster-config-condition branch from 5862378 to 5535e7f Compare October 24, 2024 13:49
@YutaroHayakawa YutaroHayakawa marked this pull request as ready for review October 24, 2024 13:49
@YutaroHayakawa YutaroHayakawa requested review from a team as code owners October 24, 2024 13:50
@YutaroHayakawa YutaroHayakawa force-pushed the yutaro/bgp-cluster-config-condition branch from 5535e7f to 812070d Compare October 24, 2024 14:27
@YutaroHayakawa
Copy link
Member Author

/test

harsimran-pabla
harsimran-pabla approved these changes Oct 24, 2024
View reviewed changes
Copy link
Contributor

@harsimran-pabla harsimran-pabla left a comment

Choose a reason for hiding this comment

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

Thanks, great change 🚀

harsimran-pabla
harsimran-pabla reviewed Oct 24, 2024
View reviewed changes
youngnick
youngnick approved these changes Oct 24, 2024
View reviewed changes
Copy link
Contributor

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

Aside from checking for changes before pushing status updates, LGTM.

@YutaroHayakawa
bgpv2: Introduce NoMatchingNode condition
6add65e 
Introduce a new condition NoMatchingNode which indicates the
spec.nodeSelector selects nothing.

Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
@YutaroHayakawa YutaroHayakawa force-pushed the yutaro/bgp-cluster-config-condition branch from 812070d to 6add65e Compare October 25, 2024 05:33
@YutaroHayakawa
Copy link
Member Author

/test

1 similar comment
@YutaroHayakawa
Copy link
Member Author

/test

rastislavs
rastislavs approved these changes Oct 25, 2024
View reviewed changes
Copy link
Contributor

@rastislavs rastislavs left a comment

Choose a reason for hiding this comment

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

Very nice, thanks!

@YutaroHayakawa YutaroHayakawa added this pull request to the merge queue Oct 25, 2024
Merged via the queue into main with commit 0f438cf Oct 25, 2024
344 of 457 checks passed
@YutaroHayakawa YutaroHayakawa deleted the yutaro/bgp-cluster-config-condition branch October 25, 2024 10:42
@YutaroHayakawa YutaroHayakawa mentioned this pull request Oct 29, 2024
Merged
@joestringer
Copy link
Member

I see this was labeled as release-note/misc, but it's changing APIs. Should this be labeled at least as release-note/minor? We also may want to think about whether to increase the version (eg v1alpha1 -> v1alpha2) when making changes to the APIs. Not sure if that strictly applies when only adding new fields like status, but we have had complaints from users recently when making changes to existing APIs (even alpha ones) since users do not expect a specific version of an API to change---that's why versions exist 😅 .

@harsimran-pabla harsimran-pabla mentioned this pull request Nov 4, 2024
Closed
3 tasks
@YutaroHayakawa YutaroHayakawa added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. and removed release-note/misc This PR makes changes that have no direct user impact. labels Nov 5, 2024
@YutaroHayakawa
Copy link
Member Author

YutaroHayakawa commented Nov 5, 2024 •
edited
Loading

I see this was labeled as release-note/misc, but it's changing APIs. Should this be labeled at least as release-note/minor?

I thought it is just an troubleshooting improvements, so it applies to This PR makes changes that have no direct user impact.. However, yeah this maybe a users may find relevant to operating Cilium. as you said. Changed the label.

We also may want to think about whether to increase the version (eg v1alpha1 -> v1alpha2) when making changes to the APIs. Not sure if that strictly applies when only adding new fields like status, but we have had complaints from users recently when making changes to existing APIs (even alpha ones) since users do not expect a specific version of an API to change---that's why versions exist 😅 .

What was the problem for those users? At least, this is a non-breaking change (because it's a pure add). If we care about the side effects like the increasing churn rate due to the status field, I think that problem should be solved with the opt-out flag and not the CRD version.

@joestringer
Copy link
Member

I think that for those users we did actually make a breaking change, that's fair. I'm not sure I understand @cilium/sig-k8s quite well enough to really determine the consequences if we're only adding new fields.

@YutaroHayakawa YutaroHayakawa mentioned this pull request Nov 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harsimran-pabla harsimran-pabla harsimran-pabla approved these changes

@youngnick youngnick youngnick approved these changes

@rastislavs rastislavs rastislavs approved these changes

@gandro gandro Awaiting requested review from gandro gandro is a code owner automatically assigned from cilium/helm

Assignees
No one assigned
Labels
release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@YutaroHayakawa @joestringer @youngnick @rastislavs @harsimran-pabla