Skip to content

Commit e901ab7

Browse files
rawaharsrawahars
committed
review comments : 1
Signed-off-by: Harsh Rawat <harshrawat@microsoft.com>
1 parent 691a9a6 commit e901ab7

7 files changed

Lines changed: 67 additions & 93 deletions

File tree

‎internal/controller/network/interface.go‎

Lines changed: 38 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,11 @@ package network
55
import (
66
"context"
77

8+
"github.com/Microsoft/hcsshim/hcn"
89
"github.com/Microsoft/hcsshim/internal/gcs"
10+
hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2"
11+
"github.com/Microsoft/hcsshim/internal/protocol/guestrequest"
12+
"github.com/Microsoft/hcsshim/internal/protocol/guestresource"
913
)
1014

1115
// Controller manages the network lifecycle for a single pod running inside a UVM.
@@ -20,9 +24,6 @@ type Controller interface {
2024

2125
// SetupOptions holds the configuration required to set up the network for a pod.
2226
type SetupOptions struct {
23-
// PodID is the identifier of the pod whose network is being configured.
24-
PodID string
25-
2627
// NetworkNamespace is the HCN namespace ID to attach to the guest.
2728
NetworkNamespace string
2829

@@ -37,3 +38,37 @@ type SetupOptions struct {
3738
type capabilitiesProvider interface {
3839
Capabilities() gcs.GuestDefinedCapabilities
3940
}
41+
42+
// vmNetworkManager manages adding and removing network adapters for a Utility VM.
43+
// Implemented by vmmanager.UtilityVM.
44+
type vmNetworkManager interface {
45+
// AddNIC adds a network adapter to the Utility VM. `nicID` should be a string representation of a
46+
// Windows GUID.
47+
AddNIC(ctx context.Context, nicID string, settings *hcsschema.NetworkAdapter) error
48+
49+
// RemoveNIC removes a network adapter from the Utility VM. `nicID` should be a string representation of a
50+
// Windows GUID.
51+
RemoveNIC(ctx context.Context, nicID string, settings *hcsschema.NetworkAdapter) error
52+
}
53+
54+
// linuxGuestNetworkManager exposes linux guest network operations.
55+
// Implemented by guestmanager.Guest.
56+
type linuxGuestNetworkManager interface {
57+
// AddLCOWNetworkInterface adds a network interface to the LCOW guest.
58+
AddLCOWNetworkInterface(ctx context.Context, settings *guestresource.LCOWNetworkAdapter) error
59+
// RemoveLCOWNetworkInterface removes a network interface from the LCOW guest.
60+
RemoveLCOWNetworkInterface(ctx context.Context, settings *guestresource.LCOWNetworkAdapter) error
61+
}
62+
63+
// windowsGuestNetworkManager exposes windows guest network operations.
64+
// Implemented by guestmanager.Guest.
65+
type windowsGuestNetworkManager interface {
66+
// AddNetworkNamespace adds a network namespace to the WCOW guest.
67+
AddNetworkNamespace(ctx context.Context, settings *hcn.HostComputeNamespace) error
68+
// RemoveNetworkNamespace removes a network namespace from the WCOW guest.
69+
RemoveNetworkNamespace(ctx context.Context, settings *hcn.HostComputeNamespace) error
70+
// AddNetworkInterface adds a network interface to the WCOW guest.
71+
AddNetworkInterface(ctx context.Context, adapterID string, requestType guestrequest.RequestType, settings *hcn.HostComputeEndpoint) error
72+
// RemoveNetworkInterface removes a network interface from the WCOW guest.
73+
RemoveNetworkInterface(ctx context.Context, adapterID string, requestType guestrequest.RequestType, settings *hcn.HostComputeEndpoint) error
74+
}

‎internal/controller/network/network.go‎

Lines changed: 29 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -10,23 +10,17 @@ import (
1010
"strings"
1111
"sync"
1212

13+
"github.com/Microsoft/go-winio/pkg/guid"
1314
"github.com/Microsoft/hcsshim/hcn"
1415
"github.com/Microsoft/hcsshim/internal/log"
1516
"github.com/Microsoft/hcsshim/internal/logfields"
16-
"github.com/Microsoft/hcsshim/internal/vm/guestmanager"
17-
"github.com/Microsoft/hcsshim/internal/vm/vmmanager"
18-
19-
"github.com/Microsoft/go-winio/pkg/guid"
2017
"github.com/sirupsen/logrus"
2118
)
2219

2320
// Manager is the concrete implementation of [Controller].
2421
type Manager struct {
2522
mu sync.Mutex
2623

27-
// podID is the identifier of the pod whose network this Controller manages.
28-
podID string
29-
3024
// namespaceID is the HCN namespace ID in use after a successful Setup.
3125
namespaceID string
3226

@@ -40,13 +34,13 @@ type Manager struct {
4034
isNamespaceSupportedByGuest bool
4135

4236
// vmNetManager performs host-side NIC hot-add/remove on the UVM.
43-
vmNetManager vmmanager.NetworkManager
37+
vmNetManager vmNetworkManager
4438

4539
// linuxGuestMgr performs guest-side NIC inject/remove for LCOW.
46-
linuxGuestMgr guestmanager.LCOWNetworkManager
40+
linuxGuestMgr linuxGuestNetworkManager
4741

4842
// winGuestMgr performs guest-side NIC/namespace operations for WCOW.
49-
winGuestMgr guestmanager.WCOWNetworkManager
43+
winGuestMgr windowsGuestNetworkManager
5044

5145
// capsProvider exposes the guest's declared capabilities.
5246
// Used to check IsNamespaceAddRequestSupported.
@@ -61,9 +55,9 @@ var _ Controller = (*Manager)(nil)
6155
// This method is called from [VMController.CreateNetworkController()]
6256
// which injects the necessary dependencies.
6357
func New(
64-
vmNetManager vmmanager.NetworkManager,
65-
linuxGuestMgr guestmanager.LCOWNetworkManager,
66-
windowsGuestMgr guestmanager.WCOWNetworkManager,
58+
vmNetManager vmNetworkManager,
59+
linuxGuestMgr linuxGuestNetworkManager,
60+
windowsGuestMgr windowsGuestNetworkManager,
6761
capsProvider capabilitiesProvider,
6862
) *Manager {
6963
m := &Manager{
@@ -87,15 +81,12 @@ func New(
8781
// and hot-adds all endpoints found in that namespace.
8882
// It must be called only once; subsequent calls return an error.
8983
func (m *Manager) Setup(ctx context.Context, opts *SetupOptions) (err error) {
90-
ctx, _ = log.WithContext(ctx, logrus.WithField(logfields.Operation, "Network Setup"))
84+
ctx, _ = log.WithContext(ctx, logrus.WithField(logfields.Namespace, opts.NetworkNamespace))
9185

9286
m.mu.Lock()
9387
defer m.mu.Unlock()
9488

95-
log.G(ctx).WithFields(logrus.Fields{
96-
logfields.PodID: opts.PodID,
97-
logfields.Namespace: opts.NetworkNamespace,
98-
}).Debug("starting network setup")
89+
log.G(ctx).Debug("starting network setup")
9990

10091
// If Setup has already been called, then error out.
10192
if m.netState != StateNotConfigured {
@@ -138,19 +129,18 @@ func (m *Manager) Setup(ctx context.Context, opts *SetupOptions) (err error) {
138129
if err != nil {
139130
return fmt.Errorf("generate NIC GUID: %w", err)
140131
}
141-
if err = m.addEndpointToGuestNamespace(ctx, nicGUID.String(), endpoint, opts.PolicyBasedRouting); err != nil {
132+
// add the nicID and endpointID to the context for trace.
133+
nicCtx, _ := log.WithContext(ctx, logrus.WithFields(logrus.Fields{"vm_nic_id": nicGUID.String(), "hns_endpoint_id": endpoint.Id}))
134+
135+
if err = m.addEndpointToGuestNamespace(nicCtx, nicGUID.String(), endpoint, opts.PolicyBasedRouting); err != nil {
142136
return fmt.Errorf("add endpoint %s to guest: %w", endpoint.Name, err)
143137
}
144138
}
145139

146-
m.podID = opts.PodID
147140
m.namespaceID = hcnNamespace.Id
148141
m.netState = StateConfigured
149142

150-
log.G(ctx).WithFields(logrus.Fields{
151-
logfields.PodID: opts.PodID,
152-
logfields.Namespace: hcnNamespace.Id,
153-
}).Info("network setup completed successfully")
143+
log.G(ctx).Info("network setup completed successfully")
154144

155145
return nil
156146
}
@@ -160,16 +150,12 @@ func (m *Manager) Setup(ctx context.Context, opts *SetupOptions) (err error) {
160150
// It is idempotent: calling it when the network is already torn down or not yet
161151
// configured is a no-op.
162152
func (m *Manager) Teardown(ctx context.Context) error {
163-
ctx, _ = log.WithContext(ctx, logrus.WithField(logfields.Operation, "Network Teardown"))
153+
ctx, _ = log.WithContext(ctx, logrus.WithField(logfields.Namespace, m.namespaceID))
164154

165155
m.mu.Lock()
166156
defer m.mu.Unlock()
167157

168-
log.G(ctx).WithFields(logrus.Fields{
169-
logfields.PodID: m.podID,
170-
logfields.Namespace: m.namespaceID,
171-
"State": m.netState,
172-
}).Debug("starting network teardown")
158+
log.G(ctx).WithField("State", m.netState).Debug("starting network teardown")
173159

174160
if m.netState == StateTornDown {
175161
// Teardown is idempotent, so return nil if already torn down.
@@ -188,32 +174,34 @@ func (m *Manager) Teardown(ctx context.Context) error {
188174
// failures, then collect all errors.
189175
var teardownErrs []error
190176
for nicID, endpoint := range m.vmEndpoints {
191-
if err := m.removeEndpointFromGuestNamespace(ctx, nicID, endpoint); err != nil {
177+
// add the nicID and endpointID to the context for trace.
178+
nicCtx, _ := log.WithContext(ctx, logrus.WithFields(logrus.Fields{"vm_nic_id": nicID, "hns_endpoint_id": endpoint.Id}))
179+
180+
if err := m.removeEndpointFromGuestNamespace(nicCtx, nicID, endpoint); err != nil {
192181
teardownErrs = append(teardownErrs, fmt.Errorf("remove endpoint %s from guest: %w", endpoint.Name, err))
193182
continue // continue attempting to remove other endpoints
194183
}
195184

196185
delete(m.vmEndpoints, nicID)
197186
}
198187

199-
if err := m.removeNetNSInsideGuest(ctx, m.namespaceID); err != nil {
200-
teardownErrs = append(teardownErrs, fmt.Errorf("remove network namespace from guest: %w", err))
201-
}
202-
203188
if len(teardownErrs) > 0 {
204189
// If any errors were encountered during teardown, mark the state as invalid.
205190
m.netState = StateInvalid
206191
return errors.Join(teardownErrs...)
207192
}
208193

194+
if err := m.removeNetNSInsideGuest(ctx, m.namespaceID); err != nil {
195+
// Mark the state as invalid so that we can retry teardown.
196+
m.netState = StateInvalid
197+
return fmt.Errorf("remove network namespace from guest: %w", err)
198+
}
199+
209200
// Mark as torn down if we do not encounter any errors.
210201
// No further Setup or Teardown calls are allowed.
211202
m.netState = StateTornDown
212203

213-
log.G(ctx).WithFields(logrus.Fields{
214-
logfields.PodID: m.podID,
215-
"networkNamespace": m.namespaceID,
216-
}).Info("network teardown completed successfully")
204+
log.G(ctx).Info("network teardown completed successfully")
217205

218206
return nil
219207
}
@@ -222,7 +210,6 @@ func (m *Manager) Teardown(ctx context.Context) error {
222210
// the given namespace.
223211
// Endpoints are sorted so that those with names ending in "eth0" appear first.
224212
func (m *Manager) fetchEndpointsInNamespace(ctx context.Context, ns *hcn.HostComputeNamespace) ([]*hcn.HostComputeEndpoint, error) {
225-
ctx, _ = log.WithContext(ctx, logrus.WithField(logfields.Namespace, ns.Id))
226213
log.G(ctx).Info("fetching endpoints from the network namespace")
227214

228215
ids, err := hcn.GetNamespaceEndpointIds(ns.Id)
@@ -239,8 +226,8 @@ func (m *Manager) fetchEndpointsInNamespace(ctx context.Context, ns *hcn.HostCom
239226
}
240227

241228
// Ensure the endpoint named "eth0" is added first when multiple endpoints are present,
242-
// so it maps to eth0 inside the guest. CNI results aren't available here, so we rely
243-
// on the endpoint name suffix as a heuristic.
229+
// so it maps to eth0 inside the pod network namespace within guest.
230+
// CNI results aren't available here, so we rely on the endpoint name suffix as a heuristic.
244231
cmp := func(a, b *hcn.HostComputeEndpoint) int {
245232
if strings.HasSuffix(a.Name, "eth0") {
246233
return -1

‎internal/controller/network/network_lcow.go‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,6 @@ import (
1010
hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2"
1111
"github.com/Microsoft/hcsshim/internal/log"
1212
"github.com/Microsoft/hcsshim/internal/protocol/guestresource"
13-
14-
"github.com/sirupsen/logrus"
1513
)
1614

1715
// addNetNSInsideGuest maps a host network namespace into the guest as a managed Guest Network Namespace.
@@ -30,7 +28,6 @@ func (m *Manager) removeNetNSInsideGuest(_ context.Context, _ string) error {
3028
// addEndpointToGuestNamespace hot-adds an HCN endpoint to the UVM and,
3129
// configures it inside the LCOW guest.
3230
func (m *Manager) addEndpointToGuestNamespace(ctx context.Context, nicID string, endpoint *hcn.HostComputeEndpoint, isPolicyBasedRoutingSupported bool) error {
33-
ctx, _ = log.WithContext(ctx, logrus.WithFields(logrus.Fields{"nicID": nicID, "endpointID": endpoint.Id}))
3431
log.G(ctx).Info("adding endpoint to guest namespace")
3532

3633
// 1. Host-side hot-add.
@@ -68,7 +65,6 @@ func (m *Manager) addEndpointToGuestNamespace(ctx context.Context, nicID string,
6865
// removeEndpointFromGuestNamespace removes an endpoint from the LCOW guest
6966
// and then hot-removes the NIC from the host.
7067
func (m *Manager) removeEndpointFromGuestNamespace(ctx context.Context, nicID string, endpoint *hcn.HostComputeEndpoint) error {
71-
ctx, _ = log.WithContext(ctx, logrus.WithFields(logrus.Fields{"nicID": nicID, "endpointID": endpoint.Id}))
7268
log.G(ctx).Info("removing endpoint from guest namespace")
7369

7470
if m.isNamespaceSupportedByGuest {

‎internal/controller/network/network_wcow.go‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,6 @@ func (m *Manager) addNetNSInsideGuest(ctx context.Context, hcnNamespace *hcn.Hos
3434

3535
// removeNetNSInsideGuest removes the HCN namespace from the WCOW guest via GCS/GNS.
3636
func (m *Manager) removeNetNSInsideGuest(ctx context.Context, namespaceID string) error {
37-
ctx, _ = log.WithContext(ctx, logrus.WithField(logfields.Namespace, namespaceID))
38-
3937
if m.isNamespaceSupportedByGuest {
4038
log.G(ctx).Info("removing network namespace from guest")
4139

@@ -55,7 +53,6 @@ func (m *Manager) removeNetNSInsideGuest(ctx context.Context, namespaceID string
5553
// addEndpointToGuestNamespace wires an HCN endpoint into the WCOW guest in three steps:
5654
// pre-add (guest notification), host-side hot-add, and guest-side finalisation.
5755
func (m *Manager) addEndpointToGuestNamespace(ctx context.Context, nicID string, endpoint *hcn.HostComputeEndpoint, _ bool) error {
58-
ctx, _ = log.WithContext(ctx, logrus.WithFields(logrus.Fields{"nicID": nicID, "endpointID": endpoint.Id}))
5956
log.G(ctx).Info("adding network endpoint to guest namespace")
6057

6158
// 1. Guest pre-add: informs WCOW guest that a NIC is about to arrive.
@@ -101,7 +98,6 @@ func (m *Manager) addEndpointToGuestNamespace(ctx context.Context, nicID string,
10198
// removeEndpointFromGuestNamespace removes an endpoint from the WCOW guest and then
10299
// hot-removes the NIC from the host.
103100
func (m *Manager) removeEndpointFromGuestNamespace(ctx context.Context, nicID string, endpoint *hcn.HostComputeEndpoint) error {
104-
ctx, _ = log.WithContext(ctx, logrus.WithFields(logrus.Fields{"nicID": nicID, "endpointID": endpoint.Id}))
105101
log.G(ctx).Info("removing network endpoint from guest namespace")
106102

107103
// 1. Guest-side removal.

‎internal/vm/guestmanager/network_lcow.go‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -11,16 +11,6 @@ import (
1111
"github.com/Microsoft/hcsshim/internal/protocol/guestresource"
1212
)
1313

14-
// LCOWNetworkManager exposes guest network operations.
15-
type LCOWNetworkManager interface {
16-
// AddLCOWNetworkInterface adds a network interface to the LCOW guest.
17-
AddLCOWNetworkInterface(ctx context.Context, settings *guestresource.LCOWNetworkAdapter) error
18-
// RemoveLCOWNetworkInterface removes a network interface from the LCOW guest.
19-
RemoveLCOWNetworkInterface(ctx context.Context, settings *guestresource.LCOWNetworkAdapter) error
20-
}
21-
22-
var _ LCOWNetworkManager = (*Guest)(nil)
23-
2414
// AddLCOWNetworkInterface adds a network interface to the LCOW guest.
2515
func (gm *Guest) AddLCOWNetworkInterface(ctx context.Context, settings *guestresource.LCOWNetworkAdapter) error {
2616
modifyRequest := &hcsschema.ModifySettingRequest{

‎internal/vm/guestmanager/network_wcow.go‎

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -12,20 +12,6 @@ import (
1212
"github.com/Microsoft/hcsshim/internal/protocol/guestresource"
1313
)
1414

15-
// WCOWNetworkManager exposes guest network operations.
16-
type WCOWNetworkManager interface {
17-
// AddNetworkNamespace adds a network namespace to the WCOW guest.
18-
AddNetworkNamespace(ctx context.Context, settings *hcn.HostComputeNamespace) error
19-
// RemoveNetworkNamespace removes a network namespace from the WCOW guest.
20-
RemoveNetworkNamespace(ctx context.Context, settings *hcn.HostComputeNamespace) error
21-
// AddNetworkInterface adds a network interface to the WCOW guest.
22-
AddNetworkInterface(ctx context.Context, adapterID string, requestType guestrequest.RequestType, settings *hcn.HostComputeEndpoint) error
23-
// RemoveNetworkInterface removes a network interface from the WCOW guest.
24-
RemoveNetworkInterface(ctx context.Context, adapterID string, requestType guestrequest.RequestType, settings *hcn.HostComputeEndpoint) error
25-
}
26-
27-
var _ WCOWNetworkManager = (*Guest)(nil)
28-
2915
// AddNetworkNamespace adds a network namespace in the guest.
3016
func (gm *Guest) AddNetworkNamespace(ctx context.Context, settings *hcn.HostComputeNamespace) error {
3117
modifyRequest := &hcsschema.ModifySettingRequest{

‎internal/vm/vmmanager/network.go‎

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -12,22 +12,6 @@ import (
1212
"github.com/Microsoft/hcsshim/internal/protocol/guestrequest"
1313
)
1414

15-
// NetworkManager manages adding and removing network adapters for a Utility VM.
16-
type NetworkManager interface {
17-
// AddNIC adds a network adapter to the Utility VM. `nicID` should be a string representation of a
18-
// Windows GUID.
19-
AddNIC(ctx context.Context, nicID string, settings *hcsschema.NetworkAdapter) error
20-
21-
// RemoveNIC removes a network adapter from the Utility VM. `nicID` should be a string representation of a
22-
// Windows GUID.
23-
RemoveNIC(ctx context.Context, nicID string, settings *hcsschema.NetworkAdapter) error
24-
25-
// UpdateNIC updates the configuration of a network adapter on the Utility VM.
26-
UpdateNIC(ctx context.Context, nicID string, settings *hcsschema.NetworkAdapter) error
27-
}
28-
29-
var _ NetworkManager = (*UtilityVM)(nil)
30-
3115
func (uvm *UtilityVM) AddNIC(ctx context.Context, nicID string, settings *hcsschema.NetworkAdapter) error {
3216
request := hcsschema.ModifySettingRequest{
3317
RequestType: guestrequest.RequestTypeAdd,

0 commit comments

Comments
 (0)