Skip to content
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.

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

Fix openshift conformance test #128

Merged

Conversation

zshi-redhat
Copy link
Collaborator

No description provided.

@zshi-redhat zshi-redhat requested a review from pliurh May 8, 2021 06:02
@pliurh
Copy link
Collaborator

pliurh commented May 10, 2021

For maxUnavailable, I suggest we set the k8s default value explicitly instead of changing it to 10%

@zshi-redhat zshi-redhat force-pushed the fix-openshift-conformance-test branch from 7b10bb2 to 9f64903 Compare May 10, 2021 09:13
@zshi-redhat
Copy link
Collaborator Author

For maxUnavailable, I suggest we set the k8s default value explicitly instead of changing it to 10%

Updated to 25%

@@ -11,6 +11,8 @@ spec:
matchLabels:
app: sriov-network-config-daemon
updateStrategy:
rollingUpdate:
maxUnavailable: 25%
Copy link
Collaborator

Choose a reason for hiding this comment

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

According to https://kubernetes.io/docs/tasks/manage-daemon/update-daemon-set, the default value is 1 for daemonse.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the comments!
Updated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I tried to verify this PR by running the tests. It will fail the test unless this value is set to either 10% or 33%. So neither 25% nor 1 would work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should we set it to 33% instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to 33%.

@zshi-redhat zshi-redhat force-pushed the fix-openshift-conformance-test branch from 9f64903 to 89eabb6 Compare May 11, 2021 08:54
resources:
requests:
cpu: 100m
memory: 1000Mi
Copy link
Member

Choose a reason for hiding this comment

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

How did you determine the memory request of 1 GB for this daemonset? Seems a little large.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question, I didn't measure how much memory the config daemon would require.
My concern was to not make this smaller enough that would affect current deployment. But I agree that it's a little large.
I will make a change to reduce it to 100Mi.

Thanks @martinkennelly !

@zshi-redhat zshi-redhat force-pushed the fix-openshift-conformance-test branch from 89eabb6 to e3a296a Compare May 12, 2021 08:40
@SchSeba
Copy link
Collaborator

SchSeba commented Jun 6, 2021

Hi @zshi-redhat can you please also add it for the operator under the bundle?

@zshi-redhat zshi-redhat force-pushed the fix-openshift-conformance-test branch from c173ca6 to 4293b8c Compare June 21, 2021 04:29
Signed-off-by: Zenghui Shi <zshi@redhat.com>
@zshi-redhat
Copy link
Collaborator Author

Hi @zshi-redhat can you please also add it for the operator under the bundle?

Done

@pliurh pliurh merged commit 26f20eb into k8snetworkplumbingwg:master Jun 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants