MGMT-2454 Remove patch for unsupported etcd flag for versions >= 4.7

README.md

@@ -21,7 +21,7 @@ The Assisted Installer will:
 1. fetch the bootstrap ignition file (currently from S3 but will change soon) and utilize the MCO container for writing the configuration to disk (using once-from option).
 1. start the bootstrap services (bootkube.service, approve-csr.service, progress.service), at this point the bootstrap will start a temporary control plane.
 1. fetch the cluster kubeconfig from the assisted-service and wait for 2 master nodes to appear.
-1. patch the etcd configuration to allow etcd to start with less than 3 members.
+1. patch the etcd configuration to allow etcd to start with less than 3 members (<4.7 only).
 1. wait for 2 **ready** master nodes and for the bootkube service to complete.
 1. pivot to master by executing the master installation flow.
 
@@ -33,7 +33,6 @@ The Assisted Installer will:
 The node will start with the new CoreOS image and ignition, and will contact the machine-config-server running on the bootstrap node in order to complete the installation.
 
 # Known changes to be done:
- - Patch etcd back to it's original configuration, need to check if patch is required for OCP 4.5.0.
  - Create a machine CR for the bootstrap node in order to approve the CSR for this node.
  - Optimize install time by storing the CoreOS image on the live CD rather than downloading it from the internet.
  - Use the relevant CoreOS image for the OCP release.

deploy/assisted-installer-controller/assisted-installer-controller-pod.yaml.template

@@ -49,9 +49,8 @@ spec:
                   name: assisted-installer-controller-config
                   key: skip-cert-verification
                   optional: true
-          envFrom:
-            - configMapRef:
-                name: assisted-installer-controller-config
+            - name: OPENSHIFT_VERSION
+              value: "{{.OpenshiftVersion}}"
           {{if .CACertPath}}
           volumeMounts:
           - name: service-ca-cert-config

go.mod

@@ -10,6 +10,7 @@ require (
 	github.com/go-openapi/runtime v0.19.24
 	github.com/go-openapi/strfmt v0.19.11
 	github.com/golang/mock v1.4.4
+	github.com/hashicorp/go-version v1.2.1
 	github.com/jpillora/backoff v1.0.0
 	github.com/kelseyhightower/envconfig v1.4.0
 	github.com/metal3-io/baremetal-operator v0.0.0-20200828204955-fc35b7691a8e

go.sum

@@ -681,6 +681,8 @@ github.com/hashicorp/go-syslog v1.0.0/go.mod h1:qPfqrKkXGihmCqbJM2mZgkZGvKG1dFdv
 github.com/hashicorp/go-uuid v1.0.0/go.mod h1:6SBZvOh/SIDV7/2o3Jml5SYk/TvGqwFJ/bN7x4byOro=
 github.com/hashicorp/go-uuid v1.0.1/go.mod h1:6SBZvOh/SIDV7/2o3Jml5SYk/TvGqwFJ/bN7x4byOro=
 github.com/hashicorp/go-version v1.1.0/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09ZGVZPK5anwXA=
+github.com/hashicorp/go-version v1.2.1 h1:zEfKbn2+PDgroKdiOzqiE8rsmLqU2uwi5PB5pBJ3TkI=
+github.com/hashicorp/go-version v1.2.1/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09ZGVZPK5anwXA=
 github.com/hashicorp/go.net v0.0.1/go.mod h1:hjKkEWcCURg++eb33jQU7oqQcI9XDCnUzHA0oac0k90=
 github.com/hashicorp/golang-lru v0.5.0/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8=
 github.com/hashicorp/golang-lru v0.5.1/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8=

src/assisted_installer_controller/assisted_installer_controller.go

@@ -48,7 +48,8 @@ type ControllerConfig struct {
 	PullSecretToken      string `envconfig:"PULL_SECRET_TOKEN" required:"true"`
 	SkipCertVerification bool   `envconfig:"SKIP_CERT_VERIFICATION" required:"false" default:"false"`
 	CACertPath           string `envconfig:"CA_CERT_PATH" required:"false" default:""`
-	Namespace            string `enconfig:"NAMESPACE" required:"false" default:"assisted-installer"`
+	Namespace            string `envconfig:"NAMESPACE" required:"false" default:"assisted-installer"`
+	OpenshiftVersion     string `envconfig:"OPENSHIFT_VERSION" required:"true"`
 }
 type Controller interface {
 	WaitAndUpdateNodesStatus(status *ControllerStatus)
@@ -236,9 +237,17 @@ func (c controller) postInstallConfigs() error {
 		return errors.Errorf("Timeout while waiting router ca data")
 	}
 
-	err = utils.WaitForPredicate(WaitTimeout, GeneralWaitInterval, c.unpatchEtcd)
+	unpatch, err := utils.EtcdPatchRequired(c.ControllerConfig.OpenshiftVersion)
 	if err != nil {
-		return errors.Errorf("Timeout while trying to unpatch etcd")
+		return err
+	}
+	if unpatch {
+		err = utils.WaitForPredicate(WaitTimeout, GeneralWaitInterval, c.unpatchEtcd)
+		if err != nil {
+			return errors.Errorf("Timeout while trying to unpatch etcd")
+		}
+	} else {
+		c.log.Infof("Skipping etcd unpatch for cluster version %s", c.ControllerConfig.OpenshiftVersion)
 	}
 
 	err = utils.WaitForPredicate(WaitTimeout, GeneralWaitInterval, c.validateConsolePod)

src/assisted_installer_controller/assisted_installer_controller_test.go

@@ -112,8 +112,9 @@ var _ = Describe("installer HostRoleMaster role", func() {
 
 	Context("Waiting for 3 nodes", func() {
 		conf := ControllerConfig{
-			ClusterID: "cluster-id",
-			URL:       "https://assisted-service.com:80",
+			ClusterID:        "cluster-id",
+			URL:              "https://assisted-service.com:80",
+			OpenshiftVersion: "4.7",
 		}
 		BeforeEach(func() {
 			c = NewController(l, conf, mockops, mockbmclient, mockk8sclient)
@@ -157,8 +158,9 @@ var _ = Describe("installer HostRoleMaster role", func() {
 	})
 	Context("Waiting for 3 nodes, will appear one by one", func() {
 		conf := ControllerConfig{
-			ClusterID: "cluster-id",
-			URL:       "https://assisted-service.com:80",
+			ClusterID:        "cluster-id",
+			URL:              "https://assisted-service.com:80",
+			OpenshiftVersion: "4.7",
 		}
 		BeforeEach(func() {
 			c = NewController(l, conf, mockops, mockbmclient, mockk8sclient)
@@ -205,8 +207,9 @@ var _ = Describe("installer HostRoleMaster role", func() {
 	})
 	Context("UpdateStatusFails and then succeeds", func() {
 		conf := ControllerConfig{
-			ClusterID: "cluster-id",
-			URL:       "https://assisted-service.com:80",
+			ClusterID:        "cluster-id",
+			URL:              "https://assisted-service.com:80",
+			OpenshiftVersion: "4.7",
 		}
 		BeforeEach(func() {
 			c = NewController(l, conf, mockops, mockbmclient, mockk8sclient)
@@ -232,8 +235,9 @@ var _ = Describe("installer HostRoleMaster role", func() {
 	})
 	Context("ListNodes fails and then succeeds", func() {
 		conf := ControllerConfig{
-			ClusterID: "cluster-id",
-			URL:       "https://assisted-service.com:80",
+			ClusterID:        "cluster-id",
+			URL:              "https://assisted-service.com:80",
+			OpenshiftVersion: "4.7",
 		}
 		BeforeEach(func() {
 			c = NewController(l, conf, mockops, mockbmclient, mockk8sclient)
@@ -253,8 +257,9 @@ var _ = Describe("installer HostRoleMaster role", func() {
 	})
 	Context("validating ApproveCsrs", func() {
 		conf := ControllerConfig{
-			ClusterID: "cluster-id",
-			URL:       "https://assisted-service.com:80",
+			ClusterID:        "cluster-id",
+			URL:              "https://assisted-service.com:80",
+			OpenshiftVersion: "4.7",
 		}
 		BeforeEach(func() {
 			c = NewController(l, conf, mockops, mockbmclient, mockk8sclient)
@@ -309,8 +314,9 @@ var _ = Describe("installer HostRoleMaster role", func() {
 
 	Context("validating AddRouterCAToClusterCA", func() {
 		conf := ControllerConfig{
-			ClusterID: "cluster-id",
-			URL:       "https://assisted-service.com:80",
+			ClusterID:        "cluster-id",
+			URL:              "https://assisted-service.com:80",
+			OpenshiftVersion: "4.7",
 		}
 		BeforeEach(func() {
 			c = NewController(l, conf, mockops, mockbmclient, mockk8sclient)
@@ -368,8 +374,6 @@ var _ = Describe("installer HostRoleMaster role", func() {
 			mockbmclient.EXPECT().GetCluster(gomock.Any()).Return(&cluster, nil).Times(1)
 			mockk8sclient.EXPECT().GetConfigMap(cmNamespace, cmName).Return(&cm, nil).Times(1)
 			mockbmclient.EXPECT().UploadIngressCa(gomock.Any(), data["ca-bundle.crt"], c.ClusterID).Return(nil).Times(1)
-			mockk8sclient.EXPECT().UnPatchEtcd().Return(fmt.Errorf("dummy")).Times(1)
-			mockk8sclient.EXPECT().UnPatchEtcd().Return(nil).Times(1)
 			mockk8sclient.EXPECT().GetPods(consoleNamespace, gomock.Any(), "").Return(nil, fmt.Errorf("dummy")).Times(1)
 			mockk8sclient.EXPECT().GetPods(consoleNamespace, gomock.Any(), "").Return([]v1.Pod{{Status: v1.PodStatus{Phase: "Pending"}}}, nil).Times(1)
 			mockk8sclient.EXPECT().GetPods(consoleNamespace, gomock.Any(), "").Return([]v1.Pod{{Status: v1.PodStatus{Phase: "Running"}}}, nil).Times(1)
@@ -404,8 +408,9 @@ var _ = Describe("installer HostRoleMaster role", func() {
 
 	Context("update BMHs", func() {
 		conf := ControllerConfig{
-			ClusterID: "cluster-id",
-			URL:       "https://assisted-service.com:80",
+			ClusterID:        "cluster-id",
+			URL:              "https://assisted-service.com:80",
+			OpenshiftVersion: "4.7",
 		}
 		t := metav1.Unix(98754, 0)
 		bmhStatus := metal3v1alpha1.BareMetalHostStatus{
@@ -485,9 +490,10 @@ var _ = Describe("installer HostRoleMaster role", func() {
 
 	Context("Upload logs", func() {
 		conf := ControllerConfig{
-			ClusterID: "cluster-id",
-			URL:       "https://assisted-service.com:80",
-			Namespace: "assisted-installer",
+			ClusterID:        "cluster-id",
+			URL:              "https://assisted-service.com:80",
+			Namespace:        "assisted-installer",
+			OpenshiftVersion: "4.7",
 		}
 		var pod v1.Pod
 		BeforeEach(func() {
@@ -535,9 +541,10 @@ var _ = Describe("installer HostRoleMaster role", func() {
 
 	Context("Upload logs with oc must-gather", func() {
 		conf := ControllerConfig{
-			ClusterID: "cluster-id",
-			URL:       "https://assisted-service.com:80",
-			Namespace: "assisted-installer",
+			ClusterID:        "cluster-id",
+			URL:              "https://assisted-service.com:80",
+			Namespace:        "assisted-installer",
+			OpenshiftVersion: "4.7",
 		}
 
 		var pod v1.Pod

src/installer/installer.go

@@ -275,10 +275,19 @@ func (i *installer) downloadHostIgnition() (string, error) {
 func (i *installer) waitForControlPlane(ctx context.Context, kc k8s_client.K8SClient) error {
 	i.waitForMasterNodes(ctx, minMasterNodes, kc)
 
-	if err := kc.PatchEtcd(); err != nil {
+	patch, err := utils.EtcdPatchRequired(i.Config.OpenshiftVersion)
+	if err != nil {
 		i.log.Error(err)
 		return err
 	}
+	if patch {
+		if err := kc.PatchEtcd(); err != nil {
+			i.log.Error(err)
+			return err
+		}
+	} else {
+		i.log.Infof("Skipping etcd patch for cluster version %s", i.Config.OpenshiftVersion)
+	}
 
 	i.waitForBootkube(ctx)
 

src/installer/installer_test.go

@@ -45,7 +45,7 @@ var _ = Describe("installer HostRoleMaster role", func() {
 		installerObj       *installer
 		hostId             = "host-id"
 		bootstrapIgn       = "bootstrap.ign"
-		openShiftVersion   = "4.4"
+		openShiftVersion   = "4.7"
 		inventoryNamesHost map[string]inventory_client.HostData
 		kubeNamesIds       map[string]string
 	)
@@ -165,9 +165,6 @@ var _ = Describe("installer HostRoleMaster role", func() {
 			mockk8sclient.EXPECT().ListMasterNodes().Return(GetKubeNodes(kubeNamesIds), nil).Times(1)
 			mockbmclient.EXPECT().UpdateHostInstallProgress(gomock.Any(), inventoryNamesHost["node1"].Host.ID.String(), models.HostStageJoined, "").Times(1)
 		}
-		patchEtcdSuccess := func() {
-			mockk8sclient.EXPECT().PatchEtcd().Return(nil).Times(1)
-		}
 		prepareControllerSuccess := func() {
 			mockops.EXPECT().PrepareController().Return(nil).Times(1)
 		}
@@ -211,7 +208,6 @@ var _ = Describe("installer HostRoleMaster role", func() {
 			restartNetworkManager(nil)
 			prepareControllerSuccess()
 			startServicesSuccess()
-			patchEtcdSuccess()
 			WaitMasterNodesSucccess()
 			waitForBootkubeSuccess()
 			bootkubeStatusSuccess()
@@ -225,25 +221,6 @@ var _ = Describe("installer HostRoleMaster role", func() {
 			ret := installerObj.InstallNode()
 			Expect(ret).Should(BeNil())
 		})
-		It("bootstrap role fail", func() {
-			updateProgressSuccess([][]string{{string(models.HostStageStartingInstallation), conf.Role},
-				{string(models.HostStageWaitingForControlPlane)},
-				{string(models.HostStageInstalling), string(models.HostRoleMaster)},
-				{string(models.HostStageWritingImageToDisk)},
-			})
-			bootstrapSetup()
-			restartNetworkManager(nil)
-			prepareControllerSuccess()
-			startServicesSuccess()
-			WaitMasterNodesSucccess()
-			err := fmt.Errorf("Etcd patch failed")
-			mockk8sclient.EXPECT().PatchEtcd().Return(err).Times(1)
-			//HostRoleMaster flow:
-			downloadHostIgnitionSuccess(hostId, "master-host-id.ign")
-			writeToDiskSuccess(gomock.Any())
-			ret := installerObj.InstallNode()
-			Expect(ret).Should(Equal(err))
-		})
 		It("bootstrap role creating SSH manifest failed", func() {
 			updateProgressSuccess([][]string{{string(models.HostStageStartingInstallation), conf.Role},
 				{string(models.HostStageWaitingForControlPlane)},
@@ -276,7 +253,6 @@ var _ = Describe("installer HostRoleMaster role", func() {
 			restartNetworkManager(nil)
 			prepareControllerSuccess()
 			startServicesSuccess()
-			patchEtcdSuccess()
 			WaitMasterNodesSucccess()
 			waitForBootkubeSuccess()
 			bootkubeStatusSuccess()

src/ops/ops.go

@@ -283,8 +283,9 @@ func (o *ops) renderControllerSecret() error {
 
 func (o *ops) renderControllerPod() error {
 	var params = map[string]interface{}{
-		"ControllerImage": config.GlobalConfig.ControllerImage,
-		"CACertPath":      config.GlobalConfig.CACertPath,
+		"ControllerImage":  config.GlobalConfig.ControllerImage,
+		"CACertPath":       config.GlobalConfig.CACertPath,
+		"OpenshiftVersion": config.GlobalConfig.OpenshiftVersion,
 	}
 
 	if config.GlobalConfig.ServiceIPs != "" {

src/utils/utils.go

@@ -22,9 +22,9 @@ import (
 	ignition "github.com/coreos/ignition/v2/config/v3_1"
 	"github.com/openshift/assisted-service/models"
 
-	"github.com/vincent-petithory/dataurl"
-
+	"github.com/hashicorp/go-version"
 	"github.com/sirupsen/logrus"
+	"github.com/vincent-petithory/dataurl"
 )
 
 var (
@@ -230,3 +230,16 @@ func GenerateRequestContext() context.Context {
 func RequestIDLogger(ctx context.Context, log *logrus.Logger) logrus.FieldLogger {
 	return requestid.RequestIDLogger(log, requestid.FromContext(ctx))
 }
+
+func EtcdPatchRequired(openshiftVersion string) (bool, error) {
+	clusterVersion, err := version.NewVersion(openshiftVersion)
+	if err != nil {
+		return false, err
+	}
+	v47, err := version.NewVersion("4.7")
+	if err != nil {
+		return false, err
+	}
+
+	return clusterVersion.LessThan(v47), nil
+}

src/utils/utils_test.go

@@ -116,3 +116,51 @@ var _ = Describe("Verify_utils", func() {
 		})
 	})
 })
+
+var _ = Describe("EtcdPatchRequired", func() {
+	It("is true for versions < 4.7", func() {
+		patch, err := EtcdPatchRequired("4.6")
+		Expect(err).NotTo(HaveOccurred())
+		Expect(patch).To(BeTrue())
+
+		patch, err = EtcdPatchRequired("4.6.0")
+		Expect(err).NotTo(HaveOccurred())
+		Expect(patch).To(BeTrue())
+
+		patch, err = EtcdPatchRequired("4.6.9")
+		Expect(err).NotTo(HaveOccurred())
+		Expect(patch).To(BeTrue())
+
+		patch, err = EtcdPatchRequired("4.6.10")
+		Expect(err).NotTo(HaveOccurred())
+		Expect(patch).To(BeTrue())
+	})
+
+	It("is false for versions >= 4.7", func() {
+		patch, err := EtcdPatchRequired("4.7")
+		Expect(err).NotTo(HaveOccurred())
+		Expect(patch).To(BeFalse())
+
+		patch, err = EtcdPatchRequired("4.7.0")
+		Expect(err).NotTo(HaveOccurred())
+		Expect(patch).To(BeFalse())
+
+		patch, err = EtcdPatchRequired("4.7.5")
+		Expect(err).NotTo(HaveOccurred())
+		Expect(patch).To(BeFalse())
+
+		patch, err = EtcdPatchRequired("4.8")
+		Expect(err).NotTo(HaveOccurred())
+		Expect(patch).To(BeFalse())
+	})
+
+	It("returns an error for an empty version", func() {
+		_, err := EtcdPatchRequired("")
+		Expect(err).To(HaveOccurred())
+	})
+
+	It("returns an error for a malformed version", func() {
+		_, err := EtcdPatchRequired("4.")
+		Expect(err).To(HaveOccurred())
+	})
+})