Port Count #170
Port Count #170
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
|
I know it feels like overhead, but I would be grateful if, when possible, you could add a link to a Jira card (or a BZ) to PRs, so that it's easier for the bypasser to get some context |
|
/retest |
1 similar comment
|
/retest |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
don't retest until Nova is back up on vexxhost |
iamemilio
force-pushed
the
portCount
branch
2 times, most recently
from
06883a5 to
3d8472f
Compare
|
/retest |
| portTags: net.PortTags, | ||
| vnicType: net.VNICType, | ||
| portSecurity: net.PortSecurity, | ||
| portName: fmt.Sprintf("%s-%d", name, i), |
There was a problem hiding this comment.
I'm not sure where the instance name is enforced, but could it have the following format <infra-id>-master-<n>? and consequently port name <infra-id>-master-<n>-<n>?
There was a problem hiding this comment.
hmm, I am not sure actually. Can CAPO manage control plane resources? If so, then this is a possibility. For workers the name scheme for machines is <cluster-id>-<machineset name>-<unique hash>
There was a problem hiding this comment.
Ok, it does, so this could be a problem. The way I see it, there are 2 ways to handle this:
- Change the naming scheme of all instances to follow the
<cluster-id>-<machineset name>-<unique hash>model. - Only append a number at the end of a port's name if more than one port is requested, that way the only valid usage is for worker nodes (for now)
@MaysaMacedo I am not sure if either of these would have consequences for kuryr, but if either is acceptable, I would lean towards 1.
There was a problem hiding this comment.
Ok, I thought about this, and while its ugly for now, I would rather just leave it and make it a consideration in a bz we are going to file to eventually have upstream parity with this feature based on this work happening upstream: kubernetes-sigs#778
We will fix this post FF by allowing the port names to be explicitly setable.
There was a problem hiding this comment.
We're working on moving away from relying on ports names on 4.8, so it's fine. I was just not sure is the name of that Port would be something like ostest-qxzjd-master-port-0-3 and if it that would be expected.
There was a problem hiding this comment.
yeah, that would be a possibility for the master nodes. Its ugly, but we are going to fix it by letting you specify port names when we adopt kubernetes-sigs#778
|
@iamemilio @mandre I just had a quick look in upstream CAPO and I didn't immediately see any SR-IOV support there. I wonder if we should discuss this patch upstream first to ensure that:
It would be good to at least validate design changes like this, even if we don't do a full upstream port ourselves. |
|
This approach will likely not be acceptable upstream unless they have support for OpenShift Operators. I would need to follow up with NFV to see if there is any upstream equivilent of the OpenShift SR-IOV Operator for upstream kubernetes. |
SR-IOV may require a single port interface be attached to a node for each workload (pod) that utilizes SR-IOV networking.
SR-IOV users need to be able to disable the port security. This feature allows users to disable it in the machine spec for a given network or subnet.
|
/retest |
1 similar comment
|
/retest |
In order to aviod potential races where other threads take the same ports, we have to remove the re-use logic. Fixes Bug 1943599
The directory where origin images are stored in registry.ci.openshift.org has changed.
|
/retest |
1 similar comment
|
/retest |
|
Tested in my local env, and this works fine. There seems to be an issue with the CI |
|
/retest |
1 similar comment
|
/retest |
|
@iamemilio: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
Closing in favor of #173 |
8 participants
Allows you to specify how many ports to attach to each machine created for a given network.
This patch also includes some fixes to how ports are attached to nodes:
Fixes: OSASINFRA-2364