ensure network daemon checks if machineConfigPool resource registered before attempting to pause the MachineConfigPool #350
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
relyt0925
force-pushed
the
hypershift-support
branch
from
6c1af36
to
d1f07b6
Compare
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
Pull Request Test Coverage Report for Build 3079947985
💛 - Coveralls |
|
Are you supposing that you run sriov network operator in the managment cluster or on the guest cluster? |
|
It will run in guest cluster and be installed in same fashion as it is today with operator hub: I do not think it is safe to have operator hub content be dynamically added to the management cluster as it can have different control domains in cloud providers (and in managed SRE offerings) |
|
I do not believe the go fmt issues are releated to my pr |
pkg/daemon/daemon.go
Outdated
| // In hypershift: there are no machineConfigPool resources | ||
| // if the MachineConfigPool resource is not registered in API there is no need to pause it | ||
| if err != nil || !isRegistered { | ||
| return err |
There was a problem hiding this comment.
can you add some logging when err is nil ?
relyt0925
force-pushed
the
hypershift-support
branch
from
d1f07b6
to
19b1812
Compare
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
adrianchiris
approved these changes
Aug 15, 2022
|
@adrianchiris can this be merged? Note the go build issue is unrelated to my pr |
|
@relyt0925 After internal discussion, it seems that the main way SRIOV network operator will run in Hypershift will be on the management cluster. /hold |
bn222
reviewed
Aug 16, 2022
pkg/daemon/daemon.go
Outdated
| // In hypershift: there are no machineConfigPool resources | ||
| // if the MachineConfigPool resource is not registered in API there is no need to pause it | ||
| if !isRegistered { | ||
| glog.Info("pauseMCP(): MCP resource not registered") |
There was a problem hiding this comment.
I agree that there is no need to pause it, but this isn't the only dependency on MCP that you need to properly handle to support sriov network operator in Hypershift.
There was a problem hiding this comment.
right I do think there are potentially a couple others that would need a similar check on "machineConfig" but was going to break those up into smaller prs
|
@bn222 can you point me to any internal discussion? to be clear, a user action in the guest cluster MUST NOT result in a new pod running on the mgmt cluster, OLM itself does not have coordinates to any cluster beyond the guest. are you discussing a different scenario? |
relyt0925
force-pushed
the
hypershift-support
branch
from
19b1812
to
f0ca3a7
Compare
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
|
adjusting to look at controlPlaneTopology == "External" to see if it is a hypershift based cluster. |
relyt0925
force-pushed
the
hypershift-support
branch
from
f0ca3a7
to
0b40400
Compare
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
relyt0925
force-pushed
the
hypershift-support
branch
from
0b40400
to
6e7ca37
Compare
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
relyt0925
force-pushed
the
hypershift-support
branch
from
6e7ca37
to
22abaa2
Compare
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
relyt0925
force-pushed
the
hypershift-support
branch
from
22abaa2
to
6c3f6a1
Compare
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
relyt0925
force-pushed
the
hypershift-support
branch
from
6c3f6a1
to
0c0a221
Compare
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
|
/lgtm |
relyt0925
force-pushed
the
hypershift-support
branch
from
09bf328
to
4e31a6e
Compare
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
relyt0925
force-pushed
the
hypershift-support
branch
from
4e31a6e
to
5a92e90
Compare
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
relyt0925
force-pushed
the
hypershift-support
branch
from
5a92e90
to
a3590b0
Compare
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
|
There is still a spot that MCO is called from config daemon. |
|
/remove hold |
|
Tested this on my hypershift setup, and does what it promises. /lgtm |
|
LGTM |
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
relyt0925
force-pushed
the
hypershift-support
branch
from
c5de82b
to
ef7e4a3
Compare
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
relyt0925
force-pushed
the
hypershift-support
branch
from
ef7e4a3
to
3a2ee55
Compare
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
| @@ -0,0 +1,25 @@ | |||
| package utils | |||
There was a problem hiding this comment.
should we put this file under pkg/daemon ? as its only used there.
|
|
||
| snclient := snclientset.NewForConfigOrDie(config) | ||
| kubeclient := kubernetes.NewForConfigOrDie(config) | ||
| mcclient := mcclientset.NewForConfigOrDie(config) | ||
| openshiftFlavor := utils.OpenshiftFlavorDefault |
There was a problem hiding this comment.
thats used to construct Openshift context to my understanding. can we move this logic to a "NewOpenshfitContext(config)" or similar method part of openshift_context.go ?
| @@ -178,7 +199,10 @@ func runStartCmd(cmd *cobra.Command, args []string) { | |||
| startOpts.nodeName, | |||
| snclient, | |||
| kubeclient, | |||
| mcclient, | |||
| utils.OpenshiftContext{ | |||
There was a problem hiding this comment.
i think it would be better if daemon will store a pointer to OpenshiftContext, that way its nil when cluster is not openshift. WDYT ?
|
will be going to PTO, and dont want to block on it. I have no major concerns with this PR. @relyt0925 @bn222 PTAL on latest comments. none should be a blocker IMO. |
|
@adrianchiris Just for the record, you are ok if we don't have pointer and instead embed the struct for now? |
|
tested with image and everything looks great |
|
LGTM |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In hypershift: there are no machineConfigPool resources registered and therefore no need to pause the MachineConfigPool resource when sriov-network-operator is orchestrating sr-iov configuration. This adds a check to see if that api resource is registered with the cluster before attempting to pause the machineConfigPool resource