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

Set CLUSTER_TYPE env variable in manifests #62

Merged
merged 1 commit into from
Mar 23, 2021

Conversation

jcaamano
Copy link
Contributor

@jcaamano jcaamano commented Feb 11, 2021

Add CLUSTER_TYPE to the manifests so it can be customized without the
need to rebuild the images.

fixes: #48

@moshe010
Copy link
Collaborator

can you also undo my bad fix #52?
I was talking about this with @zshi-redhat if we can just auto detect the cluster type. He suggested
looking for that node label node.openshift.io/os_id on the master the operartor can publish the cluster type to the daemons using SriovNetworkNodeState crd
WDYT?

@jcaamano
Copy link
Contributor Author

Nothing to undo for #52 as it is not merged?

looking for that node label node.openshift.io/os_id on the master the operartor can publish the cluster type to the daemons using SriovNetworkNodeState crd
WDYT?

I would not be able to test this as I don't work with openshift. In the meantime I think that this change alleviates the issue.

@fedepaol
Copy link
Collaborator

Re: autodiscovery if the underlying cluster is openshift or vanilla kubernetes, this is how it's addressed in another operator with similar need.

https://github.com/gatekeeper/gatekeeper-operator/blob/d4345b21714b47ec4ce3423b471df99aeac203c4/main.go#L78

cc @zshi-redhat

@moshe010
Copy link
Collaborator

@jcaamano - sorry I put the wrong commit number any I push PR to revert my bad change #66.
@fedepaol - I tried to add "github.com/RHsyseng/operator-utils/pkg/utils/openshift" so we can auto descover openshift, but there are issue with go mod when I added. This is the error -
"github.com/googleapis/gnostic/OpenAPIv2: module github.com/googleapis/gnostic@latest found (v0.5.4), but does not contain package github.com/googleapis/gnostic/OpenAPIv2"

@jcaamano
Copy link
Contributor Author

@jcaamano - sorry I put the wrong commit number any I push PR to revert my bad change #66.

@moshe010 That needs to stay IMO. Setting CLUSTER_TYPE to kubernetes is precisely what we need for the deploy-setup-k8s target overriding the default openshift that is set on hack/env.sh

@moshe010
Copy link
Collaborator

yes but sriov-operator and the sriov daemon-config read it from ENV variable in the POD so this change doesn't have an effect and the CLUSTER_TYPE is still openshift in the POD.

@jcaamano
Copy link
Contributor Author

@moshe010 not when combined with this PR. The value set in deploy-setup-k8s in #66 will then be set in the sriov-operator POD via the change on this PR deploy/operator.yaml and:

envsubst< ${m} | ${OPERATOR_EXEC} apply ${namespace:-} --validate=false -f -
and in turn the operator will set it on the config daemon POD.

@moshe010
Copy link
Collaborator

ha I see, Let wait for @zshi-redhat to be back from the PTO and see if we would go with this approach or auto-detect openshift (which I have problem to import github.com/RHsyseng/operator-utils/pkg/utils/openshift )

@zshi-redhat
Copy link
Collaborator

ha I see, Let wait for @zshi-redhat to be back from the PTO and see if we would go with this approach or auto-detect openshift (which I have problem to import github.com/RHsyseng/operator-utils/pkg/utils/openshift )

I hit a different issue importing the RHsyseng/operator-utils:

go: github.com/RHsyseng/[email protected] requires
	github.com/openshift/[email protected]: invalid version: unknown revision 000000000000

If above issue can be fixed, I think it's better to use auto-detection of platform in operator controller and publish the cluster type in SriovOperatorConfig Status unless there is case we want to force the CLUSTER_TYPE. sriov-network-config-daemon has a SriovOperatorConfig handler which can be used to set the CLUSTER_TYPE env var in its pod.

Another thought for "detecting" platform is to use Namespace (currently we use different namespace for openshift and k8s).

We can use this PR to enable k8s CLUSTER_TYPE and have another one for auto-detection when the importing issue is fixed or namespace detection is preferred.

@zshi-redhat
Copy link
Collaborator

ha I see, Let wait for @zshi-redhat to be back from the PTO and see if we would go with this approach or auto-detect openshift (which I have problem to import github.com/RHsyseng/operator-utils/pkg/utils/openshift )

I hit a different issue importing the RHsyseng/operator-utils:

go: github.com/RHsyseng/[email protected] requires
	github.com/openshift/[email protected]: invalid version: unknown revision 000000000000

If above issue can be fixed, I think it's better to use auto-detection of platform in operator controller and publish the cluster type in SriovOperatorConfig Status unless there is case we want to force the CLUSTER_TYPE. sriov-network-config-daemon has a SriovOperatorConfig handler which can be used to set the CLUSTER_TYPE env var in its pod.

Btw, it is not straight forward to use operator-utils in sriov-network-config-daemon which relies on CLUSTER_TYPE to generate rest.Config: https://github.com/k8snetworkplumbingwg/sriov-network-operator/blob/master/cmd/sriov-network-config-daemon/start.go#L89

@moshe010
Copy link
Collaborator

@zshi-redhat so maybe current approach is good enough for now

Add CLUSTER_TYPE to the manifests so it can be customized without the
need to rebuild the images.
@martinkennelly
Copy link
Member

/LGTM. @zshi-redhat Are you ok to proceed with this PR and then autodetection later?

@zshi-redhat zshi-redhat merged commit dcf8db2 into k8snetworkplumbingwg:master Mar 23, 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
5 participants