Repository: istio/istio
File: cni/pkg/cmd/root.go (Go)

Code before:
			compiledSelectors, err := util.NewCompiledEnablementSelectors(selectors)
			if err != nil {
				return fmt.Errorf("failed to instantiate ambient enablement selector: %v", err)
			}
			ambientAgent, err := nodeagent.NewServer(ctx, watchServerReady, cniEventAddr,
				nodeagent.AmbientArgs{
					SystemNamespace:            nodeagent.SystemNamespace,
					Revision:                   nodeagent.Revision,
					ServerSocket:               cfg.InstallConfig.ZtunnelUDSAddress,
					EnablementSelector:         compiledSelectors,
					DNSCapture:                 cfg.InstallConfig.AmbientDNSCapture,
					EnableIPv6:                 cfg.InstallConfig.AmbientIPv6,
					ReconcilePodRulesOnStartup: cfg.InstallConfig.AmbientReconcilePodRulesOnStartup,
				})
			if err != nil {
				return fmt.Errorf("failed to create ambient nodeagent service: %v", err)
			}

			// Ambient watch server IS enabled - on shutdown
			// we need to check and see if this is an upgrade.
			//
			// if it is, we do NOT remove the plugin, and do
			// NOT do ambient watch server cleanup
			defer func() {
				var shouldStopCleanup bool
				if cfg.InstallConfig.AmbientDisableSafeUpgrade {
					log.Info("Ambient node agent safe upgrade explicitly disabled via env")
					shouldStopCleanup = false
				} else {
					shouldStopCleanup = ambientAgent.ShouldStopCleanup("istio-cni", nodeagent.PodNamespace)
				}
				log.Infof("Ambient node agent shutting down - is upgrade shutdown? %t", shouldStopCleanup)
				// if we are doing an "upgrade shutdown", then
				// we do NOT want to remove/cleanup the CNI plugin.
				//
				// This is important - we want it to remain in place to "stall"
				// new ambient-enabled pods while our replacement spins up.
				if !shouldStopCleanup {
					if cleanErr := installer.Cleanup(); cleanErr != nil {
						log.Error(cleanErr.Error())
					}
				}
				ambientAgent.Stop(shouldStopCleanup)
			}()

			ambientAgent.Start()

			log.Info("Ambient node agent started, starting installer...")

		} else {
			// Ambient not enabled, so this readiness flag is no-op'd

Review comment (suggestion):
not: the polarity changing here makes it difficult for me to know if the logic switch is intentional or not. Before the check was "is this an upgrade" now it's "should we stop a thing". Ideally, the polarity could stay the same 

Code after:
			compiledSelectors, err := util.NewCompiledEnablementSelectors(selectors)
			if err != nil {
				return fmt.Errorf("failed to instantiate ambient enablement selector: %v", err)
			}
			ambientAgent, err := nodeagent.NewServer(ctx, watchServerReady, cniEventAddr,
				nodeagent.AmbientArgs{
					SystemNamespace:            nodeagent.SystemNamespace,
					Revision:                   nodeagent.Revision,
					ServerSocket:               cfg.InstallConfig.ZtunnelUDSAddress,
					EnablementSelector:         compiledSelectors,
					DNSCapture:                 cfg.InstallConfig.AmbientDNSCapture,
					EnableIPv6:                 cfg.InstallConfig.AmbientIPv6,
					ReconcilePodRulesOnStartup: cfg.InstallConfig.AmbientReconcilePodRulesOnStartup,
					NativeNftables:             cfg.InstallConfig.NativeNftables,
				})
			if err != nil {
				return fmt.Errorf("failed to create ambient nodeagent service: %v", err)
			}

			// Ambient watch server IS enabled - on shutdown
			// we need to check and see if this is an upgrade.
			//
			// if it is, we do NOT remove the plugin, and do
			// NOT do ambient watch server cleanup
			defer func() {
				var shouldStopCleanup bool
				if cfg.InstallConfig.AmbientDisableSafeUpgrade {
					log.Info("Ambient node agent safe upgrade explicitly disabled via env")
					shouldStopCleanup = false
				} else {
					shouldStopCleanup = ambientAgent.ShouldStopCleanup("istio-cni", nodeagent.PodNamespace, cfg.InstallConfig.IstioOwnedCNIConfig)
				}
				log.Infof("Ambient node agent shutting down - should stop cleanup? %t", shouldStopCleanup)

				// TODO(jaellio) - do we want to add support for a partial cleanup
				// if we are doing an "upgrade shutdown", then
				// we do NOT want to remove/cleanup the CNI plugin.
				//
				// This is important - we want it to remain in place to "stall"
				// new ambient-enabled pods while our replacement spins up.
				if !shouldStopCleanup {
					if cleanErr := installer.Cleanup(); cleanErr != nil {
						log.Error(cleanErr.Error())
					}
				}
				ambientAgent.Stop(shouldStopCleanup)
			}()

			ambientAgent.Start()

			log.Info("Ambient node agent started, starting installer...")