Feature/check min agent version #314

DTMad · 2020-09-14T05:25:22Z

No description provided.

# Conflicts: # pkg/controller/oneagent/oneagent_controller.go # pkg/controller/oneagent/oneagent_utils.go # pkg/controller/oneagent/oneagent_utils_test.go

…feature/AddImmutableImage

# Conflicts: # pkg/controller/oneagent/oneagent_controller.go # pkg/controller/oneagent/oneagent_update.go # pkg/controller/utils/cluster_version.go # pkg/controller/utils/cluster_version_test.go

Co-authored-by: Michael Rynkiewicz <michaelrynkiewicz3@gmail.com>

…trace-oneagent-operator into feature/AddImmutableImage

# Conflicts: # pkg/controller/oneagent/oneagent_controller.go

lrgar · 2020-09-14T06:06:01Z

go.sum

@@ -1,56 +1,91 @@
 bazil.org/fuse v0.0.0-20160811212531-371fbbdaa898/go.mod h1:Xbm+BRKSBEpa4q4hTSxohYNQpsxXPbPry4JJWOB3LB8=


You can discard the changes to this file since there are none in go.mod.

My point here remains.

lrgar · 2020-09-14T06:17:15Z

pkg/controller/utils/immutable_image.go

+
+const updateInterval = 5 * time.Minute
+
+type Operator interface {


Is this used at all? And if so, there is reconcile.Reconcile already.

lrgar · 2020-09-14T10:20:33Z

pkg/controller/utils/immutable_image.go

+		metav1.Now().UTC().Sub(instance.GetStatus().LastClusterVersionProbeTimestamp.UTC()) > updateInterval {
+		instance.GetStatus().LastClusterVersionProbeTimestamp = metav1.Now()
+		instance.GetStatus().UseImmutableImage =
+			instance.GetSpec().UseImmutableImage &&


The upper if already checks for instance.GetSpec().UseImmutableImage to be true.

lrgar · 2020-09-14T10:23:38Z

pkg/controller/utils/immutable_image.go

+//			LastClusterVersionProbeTimestamp status is the duration of updateInterval behind
+// otherwise returns false
+func SetUseImmutableImageStatus(logger logr.Logger, instance v1alpha1.BaseOneAgent, agentVersion string, dtc dtclient.Client) bool {
+	if dtc == nil {


To be fair, we're ignoring whether the parameters can be nil pretty much everywhere.

lrgar · 2020-09-14T10:29:15Z

pkg/controller/oneagentapm/oneagentapm_controller.go

@@ -43,6 +43,7 @@ func Add(mgr manager.Manager) error {
 		dtcReconciler: &utils.DynatraceClientReconciler{
 			Client:          client,
 			UpdatePaaSToken: true,
+			UpdateAPIToken:  true,


Technically this is true only when immutable images are used. For customers upgrading they would receive errors for the missing ApiToken even if it doesn't affect them.

lrgar · 2020-09-14T10:38:10Z

pkg/version/agent_version_test.go

+func TestIsAgentVersionSupported(t *testing.T) {
+	logger := logf.ZapLoggerTo(os.Stdout, true)
+
+	t.Run("IsAgentVersionSupported", func(t *testing.T) {


Why the subtest?

lrgar · 2020-09-14T10:42:05Z

pkg/version/version_info.go

+}
+
+func extractVersion(versionString string) (*versionInfo, error) {
+	mainVersionRegex := regexp.MustCompile(`[\d]+\.[\d]+\.[\d]+`)


An example version is 1.203.12.20200914-092723 - I wonder if, for development pipelines, the first three fields remain the same, but the later changes.

Also, regexp.MustCompile is to be used as a global variable.

lrgar · 2020-09-14T10:45:38Z

pkg/version/version_info.go

+	mainVersionRegex := regexp.MustCompile(`[\d]+\.[\d]+\.[\d]+`)
+	mainVersion := mainVersionRegex.FindString(versionString)
+
+	versionMergeRegex := regexp.MustCompile(`[\d]+`)


Golang supports groups/submatches which is what you're trying to get here: https://golang.org/pkg/regexp/#Regexp.FindStringSubmatch

You could also just split the string by dot.

Changed to FindStringSubmatch.

Splitting the string by dots returns 3 values for e.g.: 1.203.0 but 4 values for e.g.: 1.203.0.20200915-101926. Opted for regex to keep that more consistent.
Furthermore the used regex checks if the values between dots are actually numbers which makes checking for a malformed version string easier

lrgar · 2020-09-14T10:54:48Z

pkg/version/agent_version.go

+	comparision, err := compareVersionInfo(agentVersion, minSupportedAgentVersion)
+	return comparision >= 0, err


Suggested change

comparision, err := compareVersionInfo(agentVersion, minSupportedAgentVersion)

return comparision >= 0, err

comparison, err := compareVersionInfo(agentVersion, minSupportedAgentVersion)

return comparison >= 0, err

lrgar · 2020-09-14T10:57:55Z

pkg/version/version_info.go

+//  n > 0: if a > b
+//  n < 0: if a < b
+//  0 with error: if a == nil || b == nil
+func compareVersionInfo(a *versionInfo, b *versionInfo) (int, error) {


I'd suggest to remove the pointers, and just assume they are correct and leave the error verification problem to an upper level when it can actually be wrong.

pointers here are used to avoid pass-by-value due to higher memory consumption

These are supposed to be executed every 5 minutes, so that's not really a problem.

… into feature/check-min-agent-version # Conflicts: # deploy/cr.yaml # go.sum # pkg/controller/oneagent/oneagent_controller.go # pkg/controller/oneagent/oneagent_update.go # pkg/webhook/bootstrapper/controller.go

…gent-version

pkg/controller/istio/controller.go

lrgar · 2020-09-16T09:16:10Z

go.sum

@@ -1,56 +1,91 @@
 bazil.org/fuse v0.0.0-20160811212531-371fbbdaa898/go.mod h1:Xbm+BRKSBEpa4q4hTSxohYNQpsxXPbPry4JJWOB3LB8=


My point here remains.

pkg/controller/nodes/nodes_controller.go

pkg/controller/nodes/nodes_controller_test.go

lrgar · 2020-09-16T09:18:37Z

pkg/version/version_info.go

+//  n > 0: if a > b
+//  n < 0: if a < b
+//  0 with error: if a == nil || b == nil
+func compareVersionInfo(a *versionInfo, b *versionInfo) (int, error) {


These are supposed to be executed every 5 minutes, so that's not really a problem.

pkg/controller/oneagent/oneagent_controller_test.go

lrgar · 2020-09-16T09:28:24Z

pkg/controller/oneagentapm/oneagentapm_controller.go

+	if instance.Spec.UseImmutableImage {
+		r.dtcReconciler.UpdateAPIToken = true
+	}
+
 	dtc, upd, err := r.dtcReconciler.Reconcile(context.TODO(), instance)


Technically r.dtcReconciler is at risk of race conditions if multiple OneAgentAPM Reconcile calls are running in parallel. I think by default running reconciliation in parallel is disabled so it's not a problem now, but I'd suggest the changes below (in part to avoid changes on global objects),

Suggested change

if instance.Spec.UseImmutableImage {

r.dtcReconciler.UpdateAPIToken = true

}

dtc, upd, err := r.dtcReconciler.Reconcile(context.TODO(), instance)

dtcRec := *r.dtcReconciler

if instance.Spec.UseImmutableImage {

dtcRec.UpdateAPIToken = true

}

dtc, upd, err := dtcRec.Reconcile(context.TODO(), instance)

lrgar · 2020-09-16T09:31:03Z

pkg/controller/oneagentapm/oneagentapm_controller_test.go

@@ -5,7 +5,7 @@ import (
 	"os"
 	"testing"

-	"github.com/Dynatrace/dynatrace-oneagent-operator/pkg/apis"
+	apis "github.com/Dynatrace/dynatrace-oneagent-operator/pkg/apis"


Suggested change

apis "github.com/Dynatrace/dynatrace-oneagent-operator/pkg/apis"

"github.com/Dynatrace/dynatrace-oneagent-operator/pkg/apis"

lrgar · 2020-09-16T09:35:06Z

pkg/dtclient/communication_hosts_test.go

@@ -166,7 +166,7 @@ func handleCommunicationHosts(request *http.Request, writer http.ResponseWriter)
 	switch request.Method {
 	case "GET":
 		writer.WriteHeader(http.StatusOK)
-		_, _ = writer.Write(commHostOutput)
+		_, _ = writer.Write([]byte(commHostOutput))


Suggested change

_, _ = writer.Write([]byte(commHostOutput))

_, _ = writer.Write(commHostOutput)

lrgar · 2020-09-16T09:36:03Z

pkg/webhook/bootstrapper/controller_test.go

+const testNamespace = "dynatrace"
+


lrgar · 2020-09-16T09:40:46Z

pkg/version/version_info.go

+	version := versionRegex.FindStringSubmatch(versionString)
+
+	if len(version) < 4 {
+		return nil, fmt.Errorf("version malformed: %s", versionString)


Just in case, things such as versionRegex.FindStringSubmatch("asdadasdsd.asd1.2.3") would pass the validation, although they most certainly won't happen.

lrgar · 2020-09-16T09:52:10Z

pkg/controller/utils/immutable_image.go

+		instance.GetStatus().LastClusterVersionProbeTimestamp = metav1.Now()
+		instance.GetStatus().UseImmutableImage =
+			version.IsRemoteClusterVersionSupported(logger, dtc) &&
+				version.IsAgentVersionSupported(logger, agentVersion)


There is also the OneAgent version configured on the cluster.

For example, customer installing Operator in a new environment using our instructions and we have .spec.useImmutableImage enabled by default in our example, their cluster supports images, but the OneAgent version they want to use (and it's configured on Dynatrace, i.e., not on .spec) doesn't, we need to disable immutable image for this case.

In fact the version on .spec is not particularly relevant since we don't modify the installer URL anyway if the version is unsupported.

This may not be an uncommon situation at the beginning once we release this feature.

Co-authored-by: Luis Garcia <luis.garcia@dynatrace.com>

…lues changed agent version string source from cr to cluster removed unnecessary aliases rolled back go.sum to master branch version

lrgar · 2020-09-17T06:41:29Z

pkg/controller/utils/immutable_image.go

+		agentVersion, err := dtc.GetLatestAgentVersion(dtclient.OsUnix, dtclient.InstallerTypeDefault)
+		if err == nil {
+			instance.GetStatus().UseImmutableImage =
+				version.IsRemoteClusterVersionSupported(logger, dtc) &&


I presume the issue with agent version failing also applies to fetching the remote cluster version, doesn't it? That is, a network glitch may disable immutable images temporarily.

DTMad and others added 30 commits September 8, 2020 09:47

Initial commit for ImmutableImage

7499674

Revert code style fixes

1ed2306

Fix test

be854e1

Change error handling

5af1b26

Use new library

b72afc8

Rebase with master

24c59b9

Add build tag

a50651f

Rollback unwanted check from master

f667029

Add customPullSecret field

00043ab

initial commit

995a36f

Merge branch 'master' into feature/AddImmutableImage

34b65ea

# Conflicts: # pkg/controller/oneagent/oneagent_controller.go # pkg/controller/oneagent/oneagent_utils.go # pkg/controller/oneagent/oneagent_utils_test.go

fixed merge

828c57e

Merge remote-tracking branch 'origin/feature/AddImmutableImage' into …

a291e95

…feature/AddImmutableImage

Merge branch 'feature/AddImmutableImage' into check-min-agent-version

cdbb0b3

# Conflicts: # pkg/controller/oneagent/oneagent_controller.go # pkg/controller/oneagent/oneagent_update.go # pkg/controller/utils/cluster_version.go # pkg/controller/utils/cluster_version_test.go

merged

2e3d70b

fixed some tests

7b8f551

Update pkg/webhook/bootstrapper/controller.go

9ea7320

Co-authored-by: Michael Rynkiewicz <michaelrynkiewicz3@gmail.com>

Adapations according to review

278387e

Merge branch 'feature/AddImmutableImage' of github.com:Dynatrace/dyna…

3878c8e

…trace-oneagent-operator into feature/AddImmutableImage

minor bugfixes

a75502d

Merge branch 'feature/AddImmutableImage' into check-min-agent-version

c367d8d

# Conflicts: # pkg/controller/oneagent/oneagent_controller.go

Added check for minimum supported oneagent version

dccc6d7

Initial commit for ImmutableImage

c6b90d8

Revert code style fixes

ef7eef8

Fix test

56074b6

Change error handling

4ee5dc2

Use new library

65360f0

Rollback unwanted check from master

fd9c2c7

initial commit

03630c0

Add customPullSecret field

f5fe963

meik99 and others added 7 commits September 11, 2020 11:14

fixed some tests

0c6c3dd

minor bugfixes

ee89071

Adapations according to review

9a80474

Added check for minimum supported oneagent version

bb753ae

Adaptions for rebase

4685a23

Adaptions after rebase

cf97018

Remove unnecessary dependency

024c3e7

DTMad requested a review from lrgar September 14, 2020 05:25

lrgar reviewed Sep 14, 2020

View reviewed changes

meik99 added 6 commits September 16, 2020 07:00

adapted for review

30191d0

Merge remote-tracking branch 'origin/master' into feature/check-min-a…

09c9d8f

…gent-version

ammended changelog

9c51177

removed e2e tests

8da0441

removed unused interface

63e8897

meik99 requested a review from lrgar September 16, 2020 07:37

lrgar reviewed Sep 16, 2020

View reviewed changes

meik99 and others added 5 commits September 16, 2020 13:42

Change error message to be inline with go convention

24b0b69

Co-authored-by: Luis Garcia <luis.garcia@dynatrace.com>

remove unnecessary alias

083cd8c

Co-authored-by: Luis Garcia <luis.garcia@dynatrace.com>

remove unnecessary alias from nodes_controller_test

c572532

Co-authored-by: Luis Garcia <luis.garcia@dynatrace.com>

remove unecessary alias from oneagent_controller_test

a360733

Co-authored-by: Luis Garcia <luis.garcia@dynatrace.com>

changed parameters for version check to call-by-value to avoid nil-va…

f549d75

…lues changed agent version string source from cr to cluster removed unnecessary aliases rolled back go.sum to master branch version

meik99 requested a review from lrgar September 17, 2020 04:16

lrgar reviewed Sep 17, 2020

View reviewed changes

changed reaction to network issue when retrieving cluster info

0303f29

meik99 requested a review from lrgar September 17, 2020 07:37

lrgar approved these changes Sep 17, 2020

View reviewed changes

meik99 merged commit cc0a3e7 into master Sep 17, 2020

DTMad deleted the feature/check-min-agent-version branch September 22, 2020 12:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/check min agent version #314

Feature/check min agent version #314

DTMad commented Sep 14, 2020

lrgar Sep 14, 2020

lrgar Sep 16, 2020

lrgar Sep 14, 2020

lrgar Sep 14, 2020

lrgar Sep 14, 2020

lrgar Sep 14, 2020

lrgar Sep 14, 2020

lrgar Sep 14, 2020

lrgar Sep 14, 2020

meik99 Sep 16, 2020

lrgar Sep 14, 2020

lrgar Sep 14, 2020

meik99 Sep 16, 2020

lrgar Sep 16, 2020

lrgar Sep 16, 2020

lrgar Sep 16, 2020

lrgar Sep 16, 2020

lrgar Sep 16, 2020

lrgar Sep 16, 2020

lrgar Sep 16, 2020

lrgar Sep 16, 2020

lrgar Sep 16, 2020

lrgar Sep 17, 2020

		@@ -1,56 +1,91 @@
		bazil.org/fuse v0.0.0-20160811212531-371fbbdaa898/go.mod h1:Xbm+BRKSBEpa4q4hTSxohYNQpsxXPbPry4JJWOB3LB8=


		const updateInterval = 5 * time.Minute

		type Operator interface {

		comparision, err := compareVersionInfo(agentVersion, minSupportedAgentVersion)
		return comparision >= 0, err

	apis "github.com/Dynatrace/dynatrace-oneagent-operator/pkg/apis"
	"github.com/Dynatrace/dynatrace-oneagent-operator/pkg/apis"

	_, _ = writer.Write([]byte(commHostOutput))
	_, _ = writer.Write(commHostOutput)

Feature/check min agent version #314

Feature/check min agent version #314

Conversation

DTMad commented Sep 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment