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

Support deploying SR-IOV network operator #58

Merged

Conversation

adrianchiris
Copy link
Collaborator

This commit adds Helm chart to sriov-network-operator.

  • Add this chart as an optional dependency to the main
    network-operator chart
  • Add the needed chart files/crds/templates
  • update README

@adrianchiris adrianchiris force-pushed the add-sriov-operator-helm-deployment branch from 6a88bfe to 5ab3557 Compare November 3, 2020 17:08
@@ -0,0 +1,112 @@
apiVersion: apiextensions.k8s.io/v1
Copy link
Collaborator

Choose a reason for hiding this comment

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

probably I don't understand helm (maybe you can show me a demo) but why do we need all the crds from SR-IOV operator?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is the intention of this chart to deploy all elements needed to use sriov network operator in a k8s cluster, so it includes its crds.

lets have an offline talk on this

@adrianchiris adrianchiris force-pushed the add-sriov-operator-helm-deployment branch 8 times, most recently from 35ef221 to 9adea9c Compare November 5, 2020 17:40

# Image URIs for sriov-network-operator components
images:
operator: adriancdockerhub/sriov-network-operator:latest
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should have sriov network operator work on vanilla k8s and have a tagged release with a container for it.

This commit adds Helm chart to sriov-network-operator.

- Add this chart as an optional dependency to the main
  network-operator chart
- Add the needed chart files/crds/templates
- update README

Signed-off-by: Adrian Chiris <adrianc@nvidia.com>
@adrianchiris adrianchiris force-pushed the add-sriov-operator-helm-deployment branch from 9adea9c to 44b6c90 Compare November 8, 2020 15:40
@@ -0,0 +1,68 @@
apiVersion: apps/v1
Copy link
Contributor

Choose a reason for hiding this comment

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

It's missing the license

description: |
SR-IOV network operator configures and manages SR-IOV networks in the kubernetes cluster

# A chart can be either an 'application' or a 'library' chart.
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-related comments

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These comments are auto generated by helm when it creates a package template, from my POV these can be left as is

# tag

resources: {}
# We usually recommend not to specify default resources and to leave this as a conscious
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-related comments

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These comments are auto generated by helm when it creates a package template, from my POV these can be left as is for the future as to ensure that resources are not explicitly specified

- name: SRIOV_NETWORK_WEBHOOK_IMAGE
value: ""
- name: RESOURCE_PREFIX
value: openshift.io
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider changing it to nvidia.com

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

+1

- name: SRIOV_CNI_IMAGE
value: {{ .Values.images.sriovCni }}
- name: SRIOV_INFINIBAND_CNI_IMAGE
value: {{ .Values.images.ibSriovCni | quote }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is | quote needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

probably not needed

Add license
Update images to official images
Change resource prefix from openshift.io to nvidia.com

Signed-off-by: Mamduh Alassi <mamduhala@mellanox.com>

# Image URIs for sriov-network-operator components
images:
operator: quay.io/openshift/origin-sriov-network-operator:4.8
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should have all sriov-network-operator related images from official project repos
e.g not from openshift

@moshe010 moshe010 changed the title [WIP] Support deploying SR-IOV network operator Support deploying SR-IOV network operator Jan 11, 2021
@moshe010 moshe010 merged commit f12f5b6 into Mellanox:master Jan 11, 2021
@moshe010 moshe010 mentioned this pull request Mar 1, 2021
26 tasks
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

3 participants