Skip to content

Conversation

@abhat
Copy link
Contributor

@abhat abhat commented Dec 11, 2020

Some users need to be able to specify the v4 and v6 join subnets
to use for ovn-k. In particular, they may be already using the
ones that ovn-k uses as default, and they need a way to be able to
specify a different set of subnets as join subnets. This PR
creates the config option for the same. Eventually this will be
consumed by the CNO using rendered bootstrap data and passed
to the ovn-k daemonsets.

Signed-off-by: Aniket Bhat anbhat@redhat.com

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

@abhat
Copy link
Contributor Author

abhat commented Dec 11, 2020

/assign @danwinship
cc @trozet

@abhat abhat changed the title Add v4 and v6 join subnets to ovn-kubernetes config Bug 1894268: Add v4 and v6 join subnets to ovn-kubernetes config Dec 11, 2020
@openshift-ci-robot openshift-ci-robot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Dec 11, 2020
@openshift-ci-robot
Copy link

@abhat: This pull request references Bugzilla bug 1894268, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.7.0) matches configured target release for branch (4.7.0)
  • bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1894268: Add v4 and v6 join subnets to ovn-kubernetes config

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.

type: integer
format: int32
minimum: 0
v4JoinSubnet:
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you consider putting them in a sub-struct? e.g. Could there be other configuration needed for this?

i.e.
ovnKubernetesConfig:
joinSubnets:
v4: 100.65.0.0/16

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't anticipate any other configuration which can fall under joinSubnets to make this as a sub-struct property.

// cluster.
// +optional
IPsecConfig *IPsecConfig `json:"ipsecConfig,omitempty"`
// v4JoinSubnet is the v4 join subnet to be used by ovn-kubernetes in case the
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what a "join subnet" is? Maybe it will be obvious to an administrator but you could add more description about that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I will add more description about what a join subnet is.

@mfojtik
Copy link
Contributor

mfojtik commented Dec 14, 2020

/hold

This is post-feature-freeze API change, might require approval from @openshift/openshift-architects

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 14, 2020
@knobunc
Copy link
Contributor

knobunc commented Dec 14, 2020

We'll hold this fix for the next release.

The join network is used inside ovn-kubernetes for the ip addresses for the logical join switches on each node. But if those addresses are not reachable outside the cluster we have effectively imposed another requirement on the operator of the cluster.

If that's the case, then I think we need to expose this, and indicate what it means (it can overlap with another cluster, the traffic will never be seen outside, but it can't be something that overlaps with a range the cluster is using, or with something the cluster will want to reach.

But I don't want to race this in to 4.7, we need to think about this.

Some users need to be able to specify the v4 and v6 join subnets
to use for ovn-k. In particular, they may be already using the
ones that ovn-k uses as default, and they need a way to be able to
specify a different set of subnets as join subnets. This commit
creates the config option for  the same. Eventually this will be
consumed by the CNO using rendered bootstrap data and passed to the
ovn-k daemonsets.

Signed-off-by: Aniket Bhat <anbhat@redhat.com>
@danwinship
Copy link
Contributor

This is really an ugly implementation detail that no user should ever have to think about. It would be better if we could find some way to not need a separate CIDR for this. Like, maybe ovn-kubernetes could just use the highest-numbered subnet from the cluster network CIDR or something like that?

@abhat
Copy link
Contributor Author

abhat commented Feb 23, 2021

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 23, 2021
@abhat
Copy link
Contributor Author

abhat commented Feb 23, 2021

@danwinship agreed it's an implementation detail that customer should not have to think about and ideally the knob should not be there. But we do allow for adding cluster CIDR and there is a potential for user-configured CIDR to step on the one we choose, unless CNO can validate addition of extra cluster CIDRs against the one we have allocated for the join subnet. Maybe all the logic can stay in CNO. wdyt?

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 23, 2021
@danwinship
Copy link
Contributor

But we do allow for adding cluster CIDR and there is a potential for user-configured CIDR to step on the one we choose, unless CNO can validate addition of extra cluster CIDRs against the one we have allocated for the join subnet.

no, I mean, if you say

clusterNetworks:
  - cidr: 10.128.0.0/14
    hostPrefix: 23

then we assign 10.128.0.0/23 to the first node, and 10.128.2.0/23 to the second node and 10.128.4.0/23 to the third node, etc; the user isn't allowed to override this. And so I'm saying, can we just pre-allocate 10.131.254.0/23 (the last /23 in 10.128.0.0/14) for the join subnet? (For new clusters; for old clusters we keep using the old value.)

@openshift-bot
Copy link

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 6, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 6, 2021

@abhat: PR needs rebase.

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.

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 6, 2021
@openshift-bot
Copy link

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

@openshift-ci openshift-ci bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 5, 2021
@openshift-ci openshift-ci bot added the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Aug 5, 2021
@openshift-bot
Copy link

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

@openshift-ci openshift-ci bot closed this Sep 5, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 5, 2021

@openshift-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

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.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 5, 2021

@abhat: This pull request references Bugzilla bug 1894268. The bug has been updated to no longer refer to the pull request using the external bug tracker.

In response to this:

Bug 1894268: Add v4 and v6 join subnets to ovn-kubernetes config

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

bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants