-
-
Notifications
You must be signed in to change notification settings - Fork 624
feat(multus-cni): Add multus-cni chart #43365
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
base: master
Are you sure you want to change the base?
Conversation
alfi0812
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not passing testing btw.
|
Should work now |
|
Does not seem like it |
The chart is rendered successfully and the daemonset is scheduled, however the init container is I believe killed before any logs are displayed. I've tested the chart on my Talos cluster with default values and it works. Could this be a problem with CI? Perhaps i should merge in master? |
|
@alfi0812 I've looked at the CI logs again. I think CI might be timing out while pulling the image, as it is quite big: $ docker image ls
i Info → U In Use
IMAGE ID DISK USAGE CONTENT SIZE EXTRA
ghcr.io/k8snetworkplumbingwg/multus-cni:v4.2.3-thick 8307ee29fc82 482MB 0B |
400mb is quite small. The problem is most likely your probes. They finish and complete before the init even runs and so the ci counts int as pass. |
Are you referring to the readiness, liveness and startup probes? If so might be misinterpreting the CI logs but I don't think that's the case because:
I said that i believe the image pull is timing out because of the events shown: There are no events after these and I believe there should be an event about the image pull succeeding when the pull finishes. |
Those probes most likely instantly pass. As you just check for the existance of a file. So the ci is finished before the rest of the containers can even start Yeah there should be more. But for CI @PrivatePuffin is the guy. The only thing i can say without a proper testing done. This cannot be merged. |
|
ALso this will definetly need more testing in general (multiple ci-values.yaml in the /ci folder). To test the important options. |
No that couldn't be happening, because that file is generated by Multus on start and removed on exit. So those probes will fail until Multus starts. |
That's fine. I can add more once we resolve the issue with CI (assuming it's CI). |
Can you try statefulset instead of daemonset? |
Yes, I can change it temporarily in order to test tonight. Out ot curiosity - why do you think that would make a difference? |
Just a random guess as our default is statefulsets. And most chart use it. |
|
@alfi0812 looks like your hunch was right. Changing the workload to a StatefulSet made CI actually start the containers. Note that the failure is because Multus did not find a primary CNI, which is required for it to work. I will leave it as a StatefulSet until CI is fixed or we are about to merge this. |
This reverts commit 2cfa39e.
e28d728 to
62e8d56
Compare
|
@alfi0812 CI now passes, however note that the chart is still set to a |
Did you see any merges regarding that? So obviously no. Is there a specific reason why you want this to be daemonset and not keep it as a statefulset? |
Yes, since it is a CNI it must run a pod on every node. Thus, it must be a |
|
Then you will have to wait or propose a fix for the ci. |
This reverts commit 6c32f19.
Description
I've been using multus for a while but I've grown frustrated with the hassle of maintaining it due to the lack of a helm chart.
This chart makes deploying and managing Multus easy!
Notable features:
⚒️ Fixes #
⚙️ Type of change
🧪 How Has This Been Tested?
I've been testing it as I have been developing the chart. I have tested the different configuration options, the uninstall mode and the examples/tutorials given in the docs page.
📃 Notes:
✔️ Checklist:
feat(chart-name):,fix(chart-name):,chore(chart-name):,docs(chart-name):orfix(docs):➕ App addition
If this PR is an app addition please make sure you have done the following.
icon.pngPlease don't blindly check all the boxes. Read them and only check those that apply.
Those checkboxes are there for the reviewer to see what is this all about and
the status of this PR with a quick glance.