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

Fill in Type field when invoking ipam DEL command #2085

Merged
merged 1 commit into from
Jun 17, 2019

Conversation

fenxiong
Copy link
Contributor

@fenxiong fenxiong commented Jun 14, 2019

Summary

After updating libcni to 0.7.0 (#2036), agent fails to invoke IPAM DEL command when cleaning up awsvpc task:

2019-06-14T17:17:30Z [WARN] Managed task [...]: failed to release ip; IPAM error: error parsing configuration: missing 'type'

This results in IPAM record not being deleted on the instance, which can ultimately lead to exhausting ip address, and when that happens, subsequent awsvpc task will fail to launch:

...error transitioning container [~internal~ecs~pause] to [RESOURCES_PROVISIONED]: container resource provisioning: failed to setup network namespace: cni setup: invoke bridge plugin failed: bridge ipam ADD: failed to execute plugin: ecs-ipam: getIPV4AddressFromDB commands: failed to get available ip from the db: getAvailableIP ipstore: failed to find available ip addresses in the subnet

It appears that starting from libcni 0.7.0, the package mandates the json blob in NetworkConfig.Bytes to have a non-empty Type field, and we are not filling in this field when invoking IPAM DEL command, so the invocation fails. This PR fill in the field to conform with what the package expects.

Implementation details

Fill in the Type field with plugin name.

Testing

New tests cover the changes: yes
Added unit test. Manually tested that the ip record is deleted in /var/lib/ecs/data/eni-ipam.db with the fix. I checked whether i can add a functional test for this, but seems like the boltdb file written by the ipam plugin can't be opened by non-root user which makes this tricky.

Description for the changelog

Bug - Fixed a bug where IPAM record was not deleted when cleaning up awsvpc task.

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Starting from libcni 0.7.0, the Type field is required. Fill in the field to conform with what the library expects.
@fenxiong fenxiong requested a review from a team June 17, 2019 16:09
@@ -404,6 +404,7 @@ func (client *cniClient) createIPAMNetworkConfig(cfg *Config) (string, *libcni.N

ipamNetworkConfig := IPAMNetworkConfig{
Name: ECSIPAMPluginName,
Type: ECSIPAMPluginName,
Copy link
Contributor

Choose a reason for hiding this comment

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

is type a free string too?

Copy link
Contributor Author

@fenxiong fenxiong Jun 17, 2019

Choose a reason for hiding this comment

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

this field is used to find the plugin binary. From the spec, "Runtimes must use the type of network (see Network Configuration below) as the name of the executable to invoke. "

@yumex93
Copy link
Contributor

yumex93 commented Jun 17, 2019

Just wanna know are we able to have functional tests to catch such kind of errors before releasing the change? Otherwise, this thing may happen again in the feature.

@fenxiong
Copy link
Contributor Author

Just wanna know are we able to have functional tests to catch such kind of errors before releasing the change? Otherwise, this thing may happen again in the feature.

"I checked whether i can add a functional test for this, but seems like the boltdb file written by the ipam plugin can't be opened by non-root user which makes this tricky."

so i'm not sure whether there's a way to check this without having to run the test as root.

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.

5 participants