Validate if the openshift master is running with mutitenant network plugin #11880
Conversation
|
Did you mean to change the file's mode? |
xiangpengzhao
force-pushed
the
validate-plugin
branch
from
ecb57aa to
a289a74
Compare
|
I changed the file's mode by mistake. Fixed. Andy, thanks for pointing it out! @ncdc |
| // TODO: Validate if the openshift master is running with mutitenant network plugin | ||
| clusterNetwork, err := p.Oclient.ClusterNetwork().Get(sdnapi.ClusterNetworkDefault) | ||
| if err != nil { | ||
| errList = append(errList, errors.New("could not manage pod network: error on getting the running network plugin")) |
There was a problem hiding this comment.
nit: 'could not manage pod network' is redundant, just throw the error msg something like 'Failed to fetch current network plugin info'
| if err != nil { | ||
| errList = append(errList, errors.New("could not manage pod network: error on getting the running network plugin")) | ||
| } else if !sdnapi.IsOpenShiftMultitenantNetworkPlugin(clusterNetwork.PluginName) { | ||
| err := fmt.Errorf("could not manage pod network: the running plugin %s is not OpenShift mutitenant network plugin", clusterNetwork.PluginName) |
There was a problem hiding this comment.
'Using plugin: %q, managing pod network is only supported for openshift multitenant network plugin'?
There was a problem hiding this comment.
Thanks @pravisankar ! I will fix it as you suggested.
| @@ -84,7 +84,14 @@ func (p *ProjectOptions) Validate() error { | |||
| errList = append(errList, errors.New("must provide --selector=<project_selector> or projects")) | |||
| } | |||
|
|
|||
| // TODO: Validate if the openshift master is running with mutitenant network plugin | |||
There was a problem hiding this comment.
When we added this CLI functionality, there is no way to fetch network plugin info from the master. Now, ClusterNetwork resource holds the plugin name which enables this validation.
@xiangpengzhao Thanks for your contribution.
There was a problem hiding this comment.
Do we expect this to be able to run against an old apiserver as well?
There was a problem hiding this comment.
Thanks for pointing it out! @liggitt .
If an old apiserver doesn't have Get ClusterNetwork API, the request will get a response with 404 code, then this CLI functionality surely will not work due to the validation. That's not what we expect. But we should not ignore this error in the validation to make the functionality run against the old apiserver, because we also get a 404 code if there is no ClusterNetwork "default" found in a new apiserver which has Get ClusterNetwork API. Is there a way to distinguish the two cases? If not, we may never has the chance to validate the plugin... Feel free to correct me if I'm wrong. Thanks!
There was a problem hiding this comment.
Another issue:
If an old apiserver supports Get ClusterNetwork API but not it's field PluginName, we also don't have a way to distinguish the two cases: a). the old apiserver; b) a new apiserver which supports PluginName but the master uses a third party network plugin. The apiserver will response an empty PluginName in both cases. :(
Should we store a fake clusternetwork info with only the plugin name when using a third party plugin?
There was a problem hiding this comment.
If the user is not using openshift-sdn plugin then "default" cluster network resource won't be present and this validation will be incorrect. So if the error returned from Get(sdnapi.ClusterNetworkDefault) is kerrs.IsNotFound or if the plugin is not openshift multitenant, we should throw an error with message 'managing pod network is only supported for openshift multitenant network plugin'.
For the second issue, AFAIK we don't expect old api server to return ClusterNetwork resource without pluginName as we only allow incremental upgrades i.e. 3.2 to 3.3 and then 3.3 to 3.4 release (#11073 (comment)). PluginName field was added in 3.3 release, so this validation in 3.4 can expect valid plugin name if present.
There was a problem hiding this comment.
@pravisankar Thanks for your comments! Addressed. PTAL.
cc @liggitt
xiangpengzhao
force-pushed
the
validate-plugin
branch
from
e38f696 to
c80761f
Compare
|
Squashed. Thanks @pravisankar |
|
[merge] |
|
Flake #11004 [merge] |
|
Evaluated for origin merge up to c80761f |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11618/) (Base Commit: ca3d987) (Image: devenv-rhel7_5396) |
If the running network plugin is not OpenShift Multitenant Network Plugin (either redhat/openshift-ovs-subnet or third party plugin), users should not manage pod network.
cc @knobunc Thanks!