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

Add validateK8sApi flag #604

Closed
wants to merge 1 commit into from

Conversation

Mmduh-483
Copy link
Contributor

@Mmduh-483 Mmduh-483 commented Jan 31, 2021

When this flag is set it validates that multus can communicate with Kubernetes API and if failed to communicate it fails the cni. If set to false then skip failure when multus can't get pod data

@coveralls
Copy link

coveralls commented Jan 31, 2021

Pull Request Test Coverage Report for Build 565848041

  • 2 of 16 (12.5%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.4%) to 70.446%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/multus/multus.go 2 16 12.5%
Totals Coverage Status
Change from base Build 552451364: -0.4%
Covered Lines: 1137
Relevant Lines: 1614

💛 - Coveralls

@Mmduh-483 Mmduh-483 changed the title Skip failure when can't communicate with kubernets api Skip communicating with kubernetes api failures Jan 31, 2021
@Mmduh-483 Mmduh-483 force-pushed the api-fail branch 3 times, most recently from 2f96a2d to a382c32 Compare February 1, 2021 08:48
@dougbtv
Copy link
Member

dougbtv commented Feb 4, 2021

We decided we'll move forward, but with making this behavior configurable.

  • types, and conf.go -- have the structures and parsing for config options
  • docs/configuration.md -- which should be updated to describe the flag

When this flag is set it validates that multus can communicate with
Kubernetes API and if failed to communicate it fails the cni.
If set to false then skip failure when multus can't get pod data

Signed-off-by: Mamduh Alassi <mamduhala@mellanox.com>
@Mmduh-483 Mmduh-483 changed the title Skip communicating with kubernetes api failures Add validateK8sApi flag Feb 14, 2021
@Mmduh-483
Copy link
Contributor Author

@dougbtv

@@ -22,6 +22,7 @@ RESTART_CRIO=false
CRIO_RESTARTED_ONCE=false
RENAME_SOURCE_CONFIG_FILE=false
SKIP_BINARY_COPY=false
VALIDATE_K8S_API=false
Copy link
Member

Choose a reason for hiding this comment

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

Why default is false?

@s1061123
Copy link
Member

Thank you so much for update PR, first.

multi-net-spec of NPWG defines that the plugin need to communicate to Kubernetes API and if cannot make it, plguin should cancel to create pod as error (see section 7.2 for details). So if we change 'skip api' is false as default, multus-cni does not following the multi-net-spec of NPWG.

I hope that multus-cni should following spec because multus-cni is the reference implementation of multi-net-spec of NPWG.

So the option should be opt-in configuration as following:

  • option name could be ignoreK8sAPI(in json) / --ignore-k8s-api (in shell script) or something like that
  • default behavior keeps as it is, 'fail (i.e. cancel to create pod) if multus cannot communicate k8s api' as spec mentioned

@dougbtv
Copy link
Member

dougbtv commented Feb 15, 2021

Yeah, let's flip that logic so the default matches the spec. And I also like the name ignoreK8sAPI.

If this is a problem, we could also bring it back up at the NPWG.

@Mmduh-483
Copy link
Contributor Author

@s1061123 @dougbtv Thanks for your comments, after internal discussion we decided to go with another approach to handle the network operator use case

@Mmduh-483 Mmduh-483 closed this Feb 16, 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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants