Skip to content

Conversation

@matmerr
Copy link
Member

@matmerr matmerr commented Jul 15, 2020

Reason for Change:

Adding an IPConfig to CNS IPAM could overwrite an existing IPConfig state. Now blocks the scenario AddIPConfig changing state from Allocated to any other state

Issue Fixed:

Requirements:

Notes:

@codecov
Copy link

codecov bot commented Jul 15, 2020

Codecov Report

Merging #614 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #614   +/-   ##
=======================================
  Coverage   38.63%   38.63%           
=======================================
  Files          48       48           
  Lines        5371     5371           
=======================================
  Hits         2075     2075           
+ Misses       3022     3021    -1     
- Partials      274      275    +1     

neaggarwMS
neaggarwMS previously approved these changes Jul 15, 2020
@neaggarwMS
Copy link
Member

Discussed offline with Mat, we need to move this API out from rest service as no external component (RequestController or CNI) should be able to update internal CNS state, Internally CNS will call this API on Create or update nc request for the new ipconfigs added in the update.

return cns.ContainerIPConfigState{
IPConfig: ipconfig,
ID: id,
NCID: ncid,
Copy link
Member

Choose a reason for hiding this comment

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

Who is going to call this API?

Copy link
Member

Choose a reason for hiding this comment

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

can we rename it to just CreateorUpdateNetworkContainer?

Copy link
Member

Choose a reason for hiding this comment

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

As discussed offline, this will require more changes to handle create and update. Lets do that change in a separate pr.

@matmerr matmerr merged commit 1d31a7f into Azure:master Jul 15, 2020
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.

2 participants