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 new guest request/resource packages #1240

Merged
merged 5 commits into from
Feb 9, 2022
Merged

Add new guest request/resource packages #1240

merged 5 commits into from
Feb 9, 2022

Conversation

anmaxvl
Copy link
Contributor

@anmaxvl anmaxvl commented Dec 4, 2021

Move guestrequest and requesttype packages from internal to
exported guestrequest and guestresource packages.

Update hcsshim and gcs to use the new guestrequest and
guestresource packages

Signed-off-by: Maksim An maksiman@microsoft.com

@anmaxvl anmaxvl force-pushed the shared-gcs-protocol branch 2 times, most recently from 7c8126b to 70bd009 Compare December 4, 2021 09:10
@anmaxvl anmaxvl changed the title Add new guestprotocol package Add new guest request/resource packages Dec 4, 2021
@anmaxvl anmaxvl force-pushed the shared-gcs-protocol branch 2 times, most recently from 01af0d4 to e68ec36 Compare December 5, 2021 07:44
@anmaxvl anmaxvl marked this pull request as ready for review December 7, 2021 18:44
@anmaxvl anmaxvl requested a review from a team as a code owner December 7, 2021 18:44
@kevpar
Copy link
Member

kevpar commented Dec 7, 2021

What's the motivation behind exporting these packages? I think we benefit from not worrying about breaking changes.

@anmaxvl
Copy link
Contributor Author

anmaxvl commented Dec 7, 2021

What's the motivation behind exporting these packages? I think we benefit from not worrying about breaking changes.

Recently I just found myself copy-pasting the same definitions in 2 places while working on PoC for hash device on a separate VHD. Additionally, after merging opengcs into hcsshim this felt like a tech debt. Maybe exporting the definitions is an overkill, but I'd at least like to make sure that gcs and hcsshim are using the same ones.

I think we benefit from not worrying about breaking changes.

You mean with the current implementation or with the proposed one?

@kevpar
Copy link
Member

kevpar commented Dec 7, 2021 •
edited
Loading

What's the motivation behind exporting these packages? I think we benefit from not worrying about breaking changes.

Recently I just found myself copy-pasting the same definitions in 2 places while working on PoC for hash device on a separate VHD. Additionally, after merging opengcs into hcsshim this felt like a tech debt. Maybe exporting the definitions is an overkill, but I'd at least like to make sure that gcs and hcsshim are using the same ones.

I think we benefit from not worrying about breaking changes.

You mean with the current implementation or with the proposed one?

With the current implementation it's all unexported so we can change whatever we want. I think merging the definitions into a single package is a great idea, but would rather not export it unless we really have to.

Basically, my hope is that no one else ever has to interact with the bridge protocol, so we can treat it as an implementation detail rather than exporting definitions.

@anmaxvl
Copy link
Contributor Author

anmaxvl commented Dec 7, 2021

What's the motivation behind exporting these packages? I think we benefit from not worrying about breaking changes.

Recently I just found myself copy-pasting the same definitions in 2 places while working on PoC for hash device on a separate VHD. Additionally, after merging opengcs into hcsshim this felt like a tech debt. Maybe exporting the definitions is an overkill, but I'd at least like to make sure that gcs and hcsshim are using the same ones.

I think we benefit from not worrying about breaking changes.

You mean with the current implementation or with the proposed one?

With the current implementation it's all unexported so we can change whatever we want. I think merging the definitions into a single package is a great idea, but would rather not export it unless we really have to.

Basically, my hope is that no one else ever has to interact with the bridge protocol, so we can treat it as an implementation detail rather than exporting definitions.

ah, yeah, that makes sense. Thanks for clarification.

msscotb
msscotb reviewed Jan 25, 2022
View reviewed changes
hcn/hcnloadbalancer.go Outdated Show resolved Hide resolved
internal/hcs/process.go Outdated Show resolved Hide resolved
msscotb
msscotb approved these changes Jan 27, 2022
View reviewed changes
Copy link
Contributor

@msscotb msscotb left a comment

Choose a reason for hiding this comment

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

lgtm

@anmaxvl anmaxvl force-pushed the shared-gcs-protocol branch 3 times, most recently from fbe4167 to ea37e5f Compare January 27, 2022 22:54
helsaawy
helsaawy approved these changes Feb 2, 2022
View reviewed changes
internal/hcs/schema2/plan9_share.go Outdated Show resolved Hide resolved
internal/hns/hnspolicylist.go Outdated Show resolved Hide resolved
@anmaxvl
Add new guest request/resource packages
41a372d 
hcsshim and GCS redefine protocol messages. Any change to
the protocol requires redifinitions in both hcsshim and GCS.
This PR combines the two protocol definitions into one to
resolve this.

Create new guestrequest and guestresource internal packages
and update references in code.

Signed-off-by: Maksim An <maksiman@microsoft.com>
@anmaxvl
pr feedback: guestrequest and guestresource as internal package
89ebebc 
Do not export guestrequest and guestresource packages and rather
keep them under internal/protocol.

Signed-off-by: Maksim An <maksiman@microsoft.com>
@anmaxvl
pr feedback: update comments to reference new guestresource package
eb71f4a 
Signed-off-by: Maksim An <maksiman@microsoft.com>
@anmaxvl
Fix rebase artifacts and tests
9762d6c 
Signed-off-by: Maksim An <maksiman@microsoft.com>
@anmaxvl
pr feedback: don't replace invalid protocol references
438fe16 
Signed-off-by: Maksim An <maksiman@microsoft.com>
@anmaxvl anmaxvl merged commit 5a3c0ef into microsoft:master Feb 9, 2022
@anmaxvl anmaxvl deleted the shared-gcs-protocol branch February 9, 2022 17:45
anmaxvl added a commit to anmaxvl/hcsshim that referenced this pull request Feb 15, 2022
@anmaxvl
Fix bugs in network setup introduced by a refactor PR
467dba1 
When consolidating guest protocol into its own package
in microsoft#1240 wrong constant
definition was used for adding a network namespace. Fix this by
using the correct constants.

Signed-off-by: Maksim An <maksiman@microsoft.com>
@anmaxvl anmaxvl mentioned this pull request Feb 15, 2022
Merged
anmaxvl added a commit that referenced this pull request Feb 15, 2022
@anmaxvl
Fix bugs in network setup introduced by a refactor PR (#1299)
12b02a1 
When consolidating guest protocol into its own package
in #1240 wrong constant
definition was used for adding a network namespace. Fix this by
using the correct constants.

Signed-off-by: Maksim An <maksiman@microsoft.com>
anmaxvl pushed a commit that referenced this pull request Feb 7, 2023
@ambarve
Merged PR 5895108: update ADO to d36cc7c
6a5be6f 
Related work items: #1240, #1241, #1258, #1260, #1270, #1271, #1272, #1279, #1286, #1288, #1289, #1292, #1293, #1294, #1295, #1297, #1298, #1299, #1301, #1305, #1307, #1308, #1309, #1310, #1313, #1314, #1315, #1318, #1320, #1321, #1322, #1324, #1326, #1328, #1332, #1333, #1334, #1335, #1337
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msscotb msscotb msscotb approved these changes

@helsaawy helsaawy helsaawy approved these changes

Assignees

@kevpar kevpar

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@anmaxvl @kevpar @msscotb @helsaawy