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 the option to auto generate 00-multus.conf #160

Merged

Conversation

Kusanagi9999
Copy link
Contributor

@Kusanagi9999 Kusanagi9999 commented Oct 3, 2018

When --multus-conf-file=auto is used, 00-multus.conf will be automatically generated from the CNI configuration file of the master plugin (the first file in lexicographical order in cni-conf-dir).

@rkamudhan @dougbtv @tomo

@Kusanagi9999 Kusanagi9999 force-pushed the add-auto-config-generation branch 2 times, most recently from 00345c2 to 3781593 Compare October 3, 2018 18:33
@coveralls
Copy link

coveralls commented Oct 3, 2018

Pull Request Test Coverage Report for Build 408

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 46.565%

Totals Coverage Status
Change from base Build 400: 0.0%
Covered Lines: 366
Relevant Lines: 786

💛 - Coveralls

Dockerfile Outdated
@@ -12,6 +12,11 @@ RUN yum install -y $INSTALL_PKGS && \
yum clean all && \
rm -rf /tmp/*

# Install jq : command-line JSON processor
RUN curl -L https://github.com/stedolan/jq/releases/download/jq-1.5/jq-linux64 > jq && \
Copy link
Member

Choose a reason for hiding this comment

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

@Kusanagi9999 -- I think for the time being if we can remove the use of jq / including the jq binary here, we could be OK to get this merged into master. Granted, it'll make the resulting config a little less human legible, but... I think that'll be OK, it'll still parse. One of my colleagues mentioned that idea of making a go application to pretty print JSON and doing a multi-stage build, but... I think that might be overkill for now.

Thank you a bunch for the pull request -- cool feature!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, sounds good. I removed jq for now.

@Kusanagi9999 Kusanagi9999 force-pushed the add-auto-config-generation branch 2 times, most recently from 4baea26 to 7d55e28 Compare October 4, 2018 21:52
@Kusanagi9999
Copy link
Contributor Author

Changed the grep expression to a more precise regex : grep -E '\.conf(list)?$'

{
"name": "multus-cni-network",
"type": "multus",
"kubeconfig": "$MULTUS_KUBECONFIG",
Copy link
Contributor Author

@Kusanagi9999 Kusanagi9999 Oct 4, 2018

Choose a reason for hiding this comment

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

This will generate Err in loading K8s Delegates k8s args: GetK8sClient: failed to get context for the kubeconfig /host/etc/cni/net.d/multus.d/multus.kubeconfig it should be /etc/cni/net.d/multus.d/multus.kubeconfig

When `--multus-conf-file=auto` is used, 00-multus.conf will be
automatically generated from the CNI configuration file of the master
plugin (the first file in lexicographical order in cni-conf-dir).
Copy link
Member

@dougbtv dougbtv left a comment

Choose a reason for hiding this comment

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

Looking good Kusanagi! Thank you for the tweaks. Let's merge it on up

@dougbtv dougbtv merged commit 46a0f75 into k8snetworkplumbingwg:master Oct 5, 2018
pliurh pushed a commit to pliurh/multus-cni that referenced this pull request Sep 22, 2023
…ion-bump

OCPBUGS-12519: Bump golang.org/x/net from 0.1.0 to 0.7.0 (k8snetworkplumbingwg#1039)
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