Skip to content

Conversation

@nddq
Copy link
Member

@nddq nddq commented Jul 31, 2022

This is the draft PR for the endpoint state management functionality for CNS for when delegated IPAM is used
TODO: Handling state for release IP config request ✅

@nddq nddq requested a review from a team as a code owner July 31, 2022 23:59
@nddq nddq requested review from rbtr and removed request for a team July 31, 2022 23:59
@nddq nddq changed the title [Draft [Draft] CNS manages endpoints state for delegate IPAM use case Jul 31, 2022
@nddq nddq added do-not-merge work-in-progress cns Related to CNS. cni Related to CNI. swift Related to SWIFT networking. labels Aug 1, 2022
@nddq nddq force-pushed the cnsEndpointMngmt branch from 3288de0 to 371be47 Compare August 5, 2022 05:08
@nddq nddq requested a review from matmerr as a code owner August 5, 2022 05:08
@nddq nddq changed the title [Draft] CNS manages endpoints state for delegate IPAM use case CNS manages endpoints state for delegate IPAM use case Aug 9, 2022
@nddq nddq force-pushed the cnsEndpointMngmt branch from 9914fca to 8dafd63 Compare August 15, 2022 20:27
defer service.Unlock()
logger.Printf("[updateEndpointState] Updating endpoint state for infra container %s", ipconfigRequest.InfraContainerID)
if endpointInfo, ok := service.EndpointState[ipconfigRequest.InfraContainerID]; ok {
logger.Printf("[updateEndpointState] Found existing endpoint state for infra container %s", ipconfigRequest.InfraContainerID)
Copy link
Member

Choose a reason for hiding this comment

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

when do we get this case? if code hits here, isnt something wrong?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have a case in mind, but it is generally better to check for duplicates in a map?

@nddq nddq changed the title CNS manages endpoints state for delegate IPAM use case feat: CNS manages endpoints state for delegate IPAM use case Aug 16, 2022
@nddq nddq requested a review from tamilmani1989 August 17, 2022 17:57
@nddq nddq force-pushed the cnsEndpointMngmt branch from 2305534 to 299d11d Compare August 17, 2022 18:07
type EndpointInfo struct {
PodName string
PodNamespace string
IfnameToIPMap map[string]*IPInfo // key : interface name, value : IPInfo
Copy link
Member

Choose a reason for hiding this comment

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

again why pointer? i guess only value will be saved here. is there any difference here if we have this as just "IPInfo"? @rbtr

Copy link
Member Author

@nddq nddq Aug 17, 2022

Choose a reason for hiding this comment

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

yeah, I only used pointer because I don't know how big the struct would grow to be. If the struct consistently stays relatively small, then I guess we don't have to use pointers.

Copy link
Member Author

Choose a reason for hiding this comment

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

actually, we need pointers for IPInfo during IPconfig request and release, as we don't want to create a new copy of the arrays within IPInfo, update with new entries, and then reassign it every time.

sync.RWMutex
dncPartitionKey string
dncPartitionKey string
EndpointState map[string]*EndpointInfo // key : container id
Copy link
Member

Choose a reason for hiding this comment

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

same as below. does it need to be pointer?

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe we don't need a pointer for this case?

@rbtr rbtr force-pushed the cnsEndpointMngmt branch from 2e95a72 to 1867cfa Compare August 22, 2022 17:08
@rbtr
Copy link
Collaborator

rbtr commented Aug 22, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rbtr rbtr merged commit 60e5a26 into Azure:master Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cni Related to CNI. cns Related to CNS. swift Related to SWIFT networking.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants