Skip to content

add a new container type for l1vh singularity container request #2215

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
Merged
merged 7 commits into from
Oct 28, 2023
Merged

add a new container type for l1vh singularity container request #2215

merged 7 commits into from
Oct 28, 2023

Conversation

@kmurudi
Copy link
Contributor

@kmurudi kmurudi commented Sep 13, 2023 •
edited
Loading

Reason for Change:

Requirements:

Notes:

@kmurudi kmurudi requested a review from a team as a code owner September 13, 2023 06:35
@kmurudi kmurudi requested a review from csfmomo September 13, 2023 06:35
@tamilmani1989 tamilmani1989 added the cns Related to CNS. label Sep 19, 2023
tamilmani1989
tamilmani1989 reviewed Sep 19, 2023
View reviewed changes
cns/NetworkContainerContract.go Outdated
AllowHostToNCCommunication bool
AllowNCToHostCommunication bool
EndpointPolicies []NetworkContainerRequestPolicies
BackendNetworkInterfaceID string
Copy link
Member

@tamilmani1989 tamilmani1989 Sep 19, 2023

Choose a reason for hiding this comment

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

can it move inside a structure which contains backendnetwork info?

Copy link
Contributor Author

@kmurudi kmurudi Sep 20, 2023

Choose a reason for hiding this comment

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

yes I can add a struct BackendNetworkInfo with these 2 new fields

Copy link
Contributor Author

@kmurudi kmurudi Sep 20, 2023

Choose a reason for hiding this comment

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

added

Copy link
Contributor Author

@kmurudi kmurudi Oct 6, 2023

Choose a reason for hiding this comment

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

@tamilmani1989 - should I be reusing the fields added by PR - 3b1c720#diff-cc7993ab0a3ea963068707fda61d4664fbac1ccf3ab3bfaf0949491d90ad7f3fR85 - only thing I'm not sure of is why PodIpInfo is being used instead of adding this to CreateNetworkContainerRequest, how are these fields sent to CNS by DNC & to be consumed by DNC? I was making this change so DNC can create a NCRequest with these fields to send to CNS..not sure if that part of change has been implemented yet for swiftv2? @rbtr

tamilmani1989
tamilmani1989 previously approved these changes Sep 22, 2023
View reviewed changes
Copy link
Member

@tamilmani1989 tamilmani1989 left a comment

Choose a reason for hiding this comment

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

lgtm but get sign off from one of dnc folks before merging

@rbtr rbtr requested a review from ramiro-gamarra September 22, 2023 21:41
ramiro-gamarra
ramiro-gamarra reviewed Sep 25, 2023
View reviewed changes
ramiro-gamarra
ramiro-gamarra previously requested changes Sep 25, 2023
View reviewed changes
Copy link
Contributor

@ramiro-gamarra ramiro-gamarra left a comment

Choose a reason for hiding this comment

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

See comment

neaggarwMS
neaggarwMS requested changes Sep 25, 2023
View reviewed changes
@kmurudi kmurudi force-pushed the Singularity_backendNCGoalState branch from c8e1ea5 to 095ff5c Compare October 4, 2023 00:20
@ramiro-gamarra ramiro-gamarra self-requested a review October 4, 2023 19:16
@ramiro-gamarra ramiro-gamarra dismissed their stale review October 12, 2023 22:51

comments resolved

kmurudi
kmurudi commented Oct 13, 2023
View reviewed changes
@kmurudi kmurudi requested a review from neaggarwMS October 19, 2023 06:33
@kmurudi kmurudi force-pushed the Singularity_backendNCGoalState branch 2 times, most recently from d7150d5 to 701346a Compare October 19, 2023 20:15
@kmurudi kmurudi requested a review from tamilmani1989 October 23, 2023 22:49
@kmurudi kmurudi force-pushed the Singularity_backendNCGoalState branch from 38ac09f to ce4f8b7 Compare October 24, 2023 04:48
neaggarwMS
neaggarwMS reviewed Oct 24, 2023
View reviewed changes
Copy link
Member

@neaggarwMS neaggarwMS left a comment

Choose a reason for hiding this comment

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

lgtm

neaggarwMS
neaggarwMS previously approved these changes Oct 24, 2023
View reviewed changes
jaer-tsun
jaer-tsun reviewed Oct 26, 2023
View reviewed changes
@kmurudi kmurudi force-pushed the Singularity_backendNCGoalState branch from ce4f8b7 to 7b2b839 Compare October 26, 2023 22:24
jaer-tsun
jaer-tsun approved these changes Oct 26, 2023
View reviewed changes
Copy link
Contributor

@jaer-tsun jaer-tsun left a comment

Choose a reason for hiding this comment

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

lgtm

@paulyufan2 paulyufan2 force-pushed the Singularity_backendNCGoalState branch from 7b2b839 to 2898688 Compare October 27, 2023 02:41
@kmurudi kmurudi enabled auto-merge (squash) October 27, 2023 17:38
@kmurudi kmurudi force-pushed the Singularity_backendNCGoalState branch from 2898688 to ab679f2 Compare October 27, 2023 18:04
@kmurudi kmurudi force-pushed the Singularity_backendNCGoalState branch from ab679f2 to 8467aad Compare October 27, 2023 21:37
@kmurudi kmurudi merged commit dda7fe8 into master Oct 28, 2023
@kmurudi kmurudi deleted the Singularity_backendNCGoalState branch October 28, 2023 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rbtr rbtr rbtr approved these changes

@neaggarwMS neaggarwMS neaggarwMS left review comments

@csfmomo csfmomo Awaiting requested review from csfmomo csfmomo is a code owner automatically assigned from Azure/acn-cns-reviewers

@ramiro-gamarra ramiro-gamarra Awaiting requested review from ramiro-gamarra

@tamilmani1989 tamilmani1989 Awaiting requested review from tamilmani1989

+1 more reviewer

@jaer-tsun jaer-tsun jaer-tsun approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cns Related to CNS.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@kmurudi @tamilmani1989 @rbtr @jaer-tsun @neaggarwMS @ramiro-gamarra