Parallel SR-IOV configuration #427
Conversation
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
Pull Request Test Coverage Report for Build 5465798889
💛 - Coveralls |
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
| if err := dn.applyDrainRequired(); err != nil { | ||
| return err | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Here we return and not continue to the OpenShift part. Seems that it can change the logic, no?
controllers/drain_controller.go
Outdated
| if err := dr.Update(ctx, &node); err != nil { | ||
| return reconcile.Result{}, err | ||
| } | ||
| drainingNodes++ |
There was a problem hiding this comment.
should we wait for annotation to be reflected back in cache ?
if, for some reason list returns items in a different order we may end up with more nodes draining than needed.
that said, im not sure this may happen (that is listing from cache will yield node list in different order).
if its the same order, then the same nodes will be selected
There was a problem hiding this comment.
since we're listening for the nodes, we shouldn't face chaching issue.
There was a problem hiding this comment.
but we can, maybe u patch one node but another node triggers reconcile.
then the new state of the first node might not have propagated to cache.
controllers/drain_controller.go
Outdated
| if utils.NodeHasAnnotation(node, "sriovnetwork.openshift.io/state", "Drain_Required") { | ||
| if drainingNodes < config.Spec.MaxParallelNodeConfiguration { | ||
| node.Annotations["sriovnetwork.openshift.io/state"] = "Draining" | ||
| if err := dr.Update(ctx, &node); err != nil { |
There was a problem hiding this comment.
add info log, that this node is now set to Draining ?
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
1 similar comment
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
controllers/drain_controller.go
Outdated
| // reconciliation count | ||
| qHandler := func(q workqueue.RateLimitingInterface) { | ||
| q.Add(reconcile.Request{NamespacedName: types.NamespacedName{ | ||
| Namespace: "drain-reconcile-namespace", |
There was a problem hiding this comment.
should the namespace by the sriov namespace?
There was a problem hiding this comment.
I used hard-coded namesapce here because it's not related to operator namespace objects
There was a problem hiding this comment.
but do we need the namespace to exist?
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
|
/test-e2e-nvidia-all |
controllers/drain_controller.go
Outdated
| } | ||
|
|
||
| // Watch for spec and annotation changes | ||
| nodePredicates := builder.WithPredicates(predicate.AnnotationChangedPredicate{}) |
There was a problem hiding this comment.
can we limit watching here on nodes that have a certain annotation ?
for nodes that dont have the node drain annot we dont need to don anything. perhaps its possible to filter them at an eariler stage and not trigger reconcile in case of annot change for those nodes.
There was a problem hiding this comment.
sounds reasonable. Let me implement own predicate for this
|
|
||
| config := &sriovnetworkv1.SriovOperatorConfig{} | ||
| err := dr.Get(ctx, types.NamespacedName{ | ||
| Name: constants.DefaultConfigName, Namespace: namespace}, config) |
There was a problem hiding this comment.
This will fail when you enqueue a reconcile request with "drain-reconcile-namespace" namespace.
perhaps you need to identify this type of request first, or enqueue with sriov operator namespace so namespace field in request will be correct.
controllers/drain_controller.go
Outdated
| } | ||
|
|
||
| reqLogger.Info("Count of draining", "drainingNodes", drainingNodes) | ||
| if config.Spec.MaxParallelNodeConfiguration != 0 && drainingNodes == config.Spec.MaxParallelNodeConfiguration { |
There was a problem hiding this comment.
nit: drainingNodes >= config.spec.MaxParallelNodeConfiguration
think of a case where we allow 5 nodes to drain, and then reduce the max number to 3 mid upgrade.
in this case we want to return as well.
| @@ -756,55 +734,14 @@ func (dn *Daemon) getNodeMachinePool() error { | |||
| return fmt.Errorf("getNodeMachinePool(): Failed to find the MCP of the node") | |||
| } | |||
|
|
|||
| func (dn *Daemon) getDrainLock(ctx context.Context, done chan bool) { | |||
| }, | ||
| }, | ||
| }) | ||
| return nil | ||
| } | ||
|
|
||
| func (dn *Daemon) pauseMCP() error { |
There was a problem hiding this comment.
next step after this, is to move pauseMCP logic to controller
|
/test-e2e-nvidia-all |
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
1 similar comment
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
|
/test-all |
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
|
/test-all |
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
|
Thanks for your PR,
To skip the vendors CIs use one of:
|
|
/test-all |
|
/test-e2e-nvidia-all |
1 similar comment
|
/test-e2e-nvidia-all |
No description provided.