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.

Sign up for GitHub

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

Jump to bottom

ADD operation now returns the IP if it has already been allocated for the same container id and interface #1

Merged
merged 4 commits into from
Jan 18, 2021
Merged

ADD operation now returns the IP if it has already been allocated for the same container id and interface #1

merged 4 commits into from
Jan 18, 2021

Conversation

zed3250
Copy link

@zed3250 zed3250 commented Jan 13, 2021

No description provided.

@zed3250 zed3250 marked this pull request as ready for review January 13, 2021 11:16
set CGO_ENABLED flag to 0.
b5f712f 
Building with the flag enabled prevents our nodes from running the
binary as it'll require glibc_2.32.
@zed3250 zed3250 self-assigned this Jan 14, 2021
kamaradclimber
kamaradclimber reviewed Jan 14, 2021
View reviewed changes
Copy link

@kamaradclimber kamaradclimber left a comment

Choose a reason for hiding this comment

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

would it make sense to publish this upstream?

@zed3250
Copy link
Author

zed3250 commented Jan 14, 2021

I'm not sure i understand

Lqp1
Lqp1 reviewed Jan 14, 2021
View reviewed changes
Copy link

@Lqp1 Lqp1 left a comment

Choose a reason for hiding this comment

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

Looks good. I have also 1 question to open discussion here:
What would be the difficulty to have a criteo-host-local in addition to host-local, so that we don't mix upstream tools which follow CNI specs with our tooling? Would this need too much copy/paste?

build_linux.sh Outdated Show resolved Hide resolved
plugins/ipam/host-local/host_local_test.go Outdated
@@ -24,7 +24,7 @@ import (

"github.com/containernetworking/cni/pkg/skel"
"github.com/containernetworking/cni/pkg/types"
"github.com/containernetworking/cni/pkg/types/020"
types020 "github.com/containernetworking/cni/pkg/types/020"
Copy link

Choose a reason for hiding this comment

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

Same, can you explain this change?

Copy link
Author

Choose a reason for hiding this comment

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

Hmm i didn't notice this one. it might be vscode that made the change

Copy link

@Lqp1 Lqp1 Jan 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

can you remove this then please?

@zed3250
Copy link
Author

zed3250 commented Jan 15, 2021

I don't think there's much difficulty. The downside i see is that we'll have a lot of duplicate code.
I believe it's best to treat this whole repo as criteo-plugins. Use the official repo If the unmodified versions are needed.

Lqp1
Lqp1 approved these changes Jan 18, 2021
View reviewed changes
Copy link

@Lqp1 Lqp1 left a comment

Choose a reason for hiding this comment

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

Looks good! You can merge it :)
If you have some time, it could be great to ask upstream what they think about either modifying the spec to allow this binary to have one option to do that, or maybe to support an other binary to behave differently.

@zed3250 zed3250 merged commit a3d3cfd into master Jan 18, 2021
@zed3250 zed3250 deleted the make-host-local-add-idempotent branch January 18, 2021 08:18
@zed3250 zed3250 restored the make-host-local-add-idempotent branch January 18, 2021 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kamaradclimber kamaradclimber kamaradclimber left review comments

@Lqp1 Lqp1 Lqp1 approved these changes

Assignees

@zed3250 zed3250

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zed3250 @kamaradclimber @Lqp1