Adding state attribute to the HNSEndpoint struct to support hyperv #2177
Conversation
ritikaguptams
force-pushed
the
hypervNetwork
branch
3 times, most recently
from
f244f58
to
c193dae
Compare
ritikaguptams
force-pushed
the
hypervNetwork
branch
3 times, most recently
from
8984865
to
4403a04
Compare
…ntainers for k8s Signed-off-by: ritikaguptams <[email protected]> Adding stringer for usage and CI/CD Signed-off-by: ritikaguptams <[email protected]> Fixing build errors Signed-off-by: ritikaguptams <[email protected]> Ignore linting for files generated by Stringer Signed-off-by: ritikaguptams <[email protected]> Trying to fix CI go gen Signed-off-by: ritikaguptams <[email protected]> Removing extra step to fix CI go gen Signed-off-by: ritikaguptams <[email protected]> go gen CI fix try 2 Signed-off-by: ritikaguptams <[email protected]> Skip autogenerated file from linting Signed-off-by: ritikaguptams <[email protected]> Fixing linting Signed-off-by: ritikaguptams <[email protected]> Fixing linting Signed-off-by: ritikaguptams <[email protected]> Removing stringer to avoid increasing package bloat for hcsshim Signed-off-by: ritikaguptams <[email protected]> cleanup Signed-off-by: ritikaguptams <[email protected]> Adding comment for future HNS v2 change Signed-off-by: ritikaguptams <[email protected]> Fix linting Signed-off-by: ritikaguptams <[email protected]>
ritikaguptams
force-pushed
the
hypervNetwork
branch
from
4403a04
to
a0122a7
Compare
| // EndpointState const | ||
| // The lifecycle of an Endpoint goes through created, attached, AttachedSharing - endpoint is being shared with other containers, | ||
| // detached, after being attached, degraded and finally destroyed. | ||
| // Note: This attribute is used by calico to define stale containers and is dependent on HNS v1 api, if we move to HNS v2 api we will need |
There was a problem hiding this comment.
@sbangari @ritikaguptams Is there a way to add a unit test that fails when we moved to v2? I think this would be really important to have to avoid any regression in the future.
There was a problem hiding this comment.
Is it that only Calico that is using the v1 endpoint? There are tests in https://github.com/microsoft/hcsshim/blob/main/hcn/hcnendpoint_test.go but I think that is v2 api?
There was a problem hiding this comment.
Hi @kiashok,
My understanding is hcsshim uses exposes both v1 and v2 apis via the hcsshim/hcn code, which is documented and has testing support. Our codeflow currently is a minor change to add an attribute in an v1 api call. Underlying understanding is calico currently runs on HNS v1 support. We should not have any breaking changes from this change as users on v2 api call would use Hcshim/hcn/hcnendpoint ListEndpointsofNetwork instead of Hcshim/hnsendpoint HNSListEndpointRequest
The comments in there are only for documentation purpose i.e. if ever calico moves to v2 they would have to redo their logic on stale endpoints, which uses 'State' as that attribute is not exposed via HNS v2 api currently.
There was a problem hiding this comment.
sounds good, thanks for the detailed response. We should open an issue on Calico project so they know they are using HNS v1 api's in some places.
The struct HNSEndpoint is used by CNIs like calico to validate the state of an endpoint. In order to support Hyperv containers on k8s we need to expose the State attribute. This attribute can be used by CNIs to determine the state of the endpoint.
HNS API versions:
Hcsshim supports both APIS v1 and v2 via the HCN folder (documented way) or via hns files in the root which expose internal/hns apis
CodeFlow:
Calico::endpoint_mgr.go::RefreshHnsEndpointCache --> hns.HNSListEndpointRequest() which lands at hcsshim/hnsendpoint.go which calls into the internal HNS package at /hcsshim/internal/hns which runs a get call into HNS OS code.