-
Notifications
You must be signed in to change notification settings - Fork 267
passt: Fix docs #916
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
passt: Fix docs #916
Conversation
|
Skipping CI for Draft Pull Request. |
|
/cc @nirdothan |
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.
Thanks @oshoval please see my comments below.
Hi, took a look, will look deeper again, but meanwhile i think we dont need to create dependecy between the 2 PRs, whatever the merge order will be, the 2nd will adapt accordingly. Thanks |
|
Pull requests that are marked with After that period the bot marks them with the label /label needs-approver-review |
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.
Thank you for the PR @oshoval!
Please see the comments inline.
| ## Passt network binding plugin | ||
| [v1.1.0] | ||
| [v1.6.0] |
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.
Could you please elaborate on the necessity of this change, considering the passt binding plugin was introduced in v1.1.0
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.
reverted
|
|
||
| And in detail: | ||
|
|
||
| ### Passt CNI deployment on nodes |
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.
Since the option to deploy the CNI via a daemonset is only available starting v1.6, I suggest that we will keep both deployment options.
Please also consider giving a few words about the relevant artifacts KubeVirt releases (CNI binary and a container image containing the CNI binary).
| kind: NetworkAttachmentDefinition | ||
| metadata: | ||
| name: netbindingpasst | ||
| name: primary-udn-kubevirt-binding |
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.
Could you please explain the rationale for this change? Additionally, the name used here is a downstream-specific term that should be avoided; let's replace it with a more generic one.
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.
reverted
| config: '{ | ||
| "cniVersion": "1.0.0", | ||
| "name": "netbindingpasst", | ||
| "name": "primary-udn-kubevirt-binding", |
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.
same.
| specified in the Kubevirt CR when registering the network binding plugin. | ||
| ### Feature Gate | ||
| If not already set, add the `NetworkBindingPlugins` FG. |
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 only relevant for KubeVirt versions < v1.5
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.
general note - i start to wonder if it wouldn't be good to have docs per release via branches
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.
+1 to having a doc per release. I see the doc now has many "version xxx and above/before version xxx"
| ### Feature Gate | ||
| If not already set, add the `NetworkBindingPlugins` FG. | ||
| If not already set, add the `PasstIPStackMigration` Feature Gate. |
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.
Could you please elaborate why should this FG be set?
Please note that this feature work only with network providers that provide sticky IPs. I would argue that this document should reflect the more common case where sticky IPs aren't necessarily provided.
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.
there is "This Feature gate toggles calling of passt-repair for Passt seamless migration.
but i will remove this change
| and [here](#passt-sidecar-image). | ||
|
|
||
| When using the plugin, additional memory overhead of `500Mi` will be requested for the compute container in the virt-launcher pod. | ||
| When using the plugin, additional memory overhead of `250Mi` will be requested for the compute container in the virt-launcher pod. |
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.
@orelmisan @nirdothan
on which version, the need was reduced to 250Mi ?
If we keep backward support in the document, it becomes important if it started since specific version
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.
on which version, the need was reduced to 250Mi ?
This is a change in passt, not KubeVirt.
This part was changed by #933, please consider rebasing.
|
thanks, addressed comments |
| The CNI plugin binary can be retrieved directly from the kubevirt release | ||
| assets (on GitHub) or to be built from its | ||
|
|
||
| #### prior to v1.6.0 |
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.
Please consider moving this section below the 'v1.6-and-above' section. It would be more logical to present the information relevant to the newest version first.
| ### Feature Gate | ||
| If not already set, add the `NetworkBindingPlugins` FG. | ||
| ### Feature Gate - for KubeVirt versions prior to v1.5 |
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.
Tis part was changed by #933, please consider rebasing.
| and [here](#passt-sidecar-image). | ||
|
|
||
| When using the plugin, additional memory overhead of `500Mi` will be requested for the compute container in the virt-launcher pod. | ||
| When using the plugin, additional memory overhead of `250Mi` will be requested for the compute container in the virt-launcher pod. |
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.
on which version, the need was reduced to 250Mi ?
This is a change in passt, not KubeVirt.
This part was changed by #933, please consider rebasing.
|
rebase / addressed comments |
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.
/lgtm
|
/cc @orelmisan for approval |
|
@frenzyfriday: GitHub didn't allow me to request PR reviews from the following users: for, approval. Note that only kubevirt members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions 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-sigs/prow repository. |
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.
Thank you for the changes @oshoval.
Just some minor comments.
| annotations: | ||
| description: passt-binding-cni installs 'passt binding' CNI on cluster nodes | ||
| spec: | ||
| priorityClassName: system-cluster-critical |
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.
nit: Do we really need to define a high priority class?
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.
on clusters that are on their resource edge, yes
it is a preserved PC, that is higher than user custom PCs
It is what we are doing on CNAO / HCO / Kubevirt on other components, it is fine to keep it
| For KubeVirt versions prior to v1.5, make sure to enable the `NetworkBindingPlugins` FG. | ||
| ``` | ||
| kubectl patch kubevirts -n kubevirt kubevirt --type=json -p='[{"op": "add", "path": "/spec/configuration/developerConfiguration/featureGates/-", "value": "NetworkBindingPlugins"}]' | ||
| kubectl patch kubevirt -n kubevirt kubevirt --type=merge -p='{"spec":{"configuration":{"developerConfiguration":{"featureGates":["NetworkBindingPlugins"]}}}}' |
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.
Could you please elaborate on the advantages of the proposed approach?
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.
the previous fails if annotation doesnt exist
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.
Which annotation?
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.
not annotation, it doesnt work if this path doesnt exists beforehand /spec/configuration/developerConfiguration/featureGates
while the new one does
if you spin kubevirt and try you will see
| > The passt binding is still in evaluation, use it with care. | ||
| **Note**: | ||
| > It is up to the cluster admin to decide if the plugin is to be available in the cluster. |
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.
I think that it is implicit since users have to manually deploy it on their clusters.
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.
same text that was before, i just removed the "The specific passt plugin has no FG by its own"
i prefer to not touch it in this PR
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.
I don't see the removal, but this is not a blocker.
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.
hmm it is double there now, will update
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.
now done
| **Note**: | ||
| > It is up to the cluster admin to decide if the plugin is to be available in the cluster. | ||
| > The passt binding is still in evaluation, use it with care. |
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.
Please consider the following alternative wording:
| > The passt binding is still in evaluation, use it with care. | |
| > The passt binding plugin is an alpha feature and may be unstable or subject to breaking changes. Use with caution. |
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.
i just changed the place, i prefer to not change it on this PR
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.
I don't see the removal, but this is not a blocker.
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.
Update docs about cni daemonSet. Update outdated parts. Signed-off-by: Or Shoval <[email protected]>
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.
Thanks @oshoval
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: orelmisan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/remove-label needs-approver-review |
What this PR does / why we need it:
Update docs about cni daemonSet.
Update outdated parts.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
Release note: