Introduced errorCode field in LastOperation struct (#851)

* add errorCode field in LastOperation struct * address review comments * address review comments part-2
gardener · Sep 18, 2023 · 5b9a383 · 5b9a383
1 parent c2decbd
commit 5b9a383
Show file tree

Hide file tree

Showing 11 changed files with 143 additions and 28 deletions.
diff --git a/docs/documents/apis.md b/docs/documents/apis.md
@@ -871,6 +871,20 @@ string
 </tr>
 <tr>
 <td>
+<code>errorCode</code>
+</td>
+<td>
+<em>
+string
+</em>
+</td>
+<td>
+<em>(Optional)</em>
+<p>ErrorCode of the current operation if any</p>
+</td>
+</tr>
+<tr>
+<td>
 <code>lastUpdateTime</code>
 </td>
 <td>

diff --git a/kubernetes/crds/machine.sapcloud.io_machinedeployments.yaml b/kubernetes/crds/machine.sapcloud.io_machinedeployments.yaml
@@ -431,6 +431,9 @@ spec:
                         description:
                           description: Description of the current operation
                           type: string
+                        errorCode:
+                          description: ErrorCode of the current operation if any
+                          type: string
                         lastUpdateTime:
                           description: Last update time of current operation
                           format: date-time

diff --git a/kubernetes/crds/machine.sapcloud.io_machines.yaml b/kubernetes/crds/machine.sapcloud.io_machines.yaml
@@ -263,6 +263,9 @@ spec:
                   description:
                     description: Description of the current operation
                     type: string
+                  errorCode:
+                    description: ErrorCode of the current operation if any
+                    type: string
                   lastUpdateTime:
                     description: Last update time of current operation
                     format: date-time

diff --git a/kubernetes/crds/machine.sapcloud.io_machinesets.yaml b/kubernetes/crds/machine.sapcloud.io_machinesets.yaml
@@ -313,6 +313,9 @@ spec:
                         description:
                           description: Description of the current operation
                           type: string
+                        errorCode:
+                          description: ErrorCode of the current operation if any
+                          type: string
                         lastUpdateTime:
                           description: Last update time of current operation
                           format: date-time
@@ -347,6 +350,9 @@ spec:
                   description:
                     description: Description of the current operation
                     type: string
+                  errorCode:
+                    description: ErrorCode of the current operation if any
+                    type: string
                   lastUpdateTime:
                     description: Last update time of current operation
                     format: date-time

diff --git a/pkg/apis/machine/types.go b/pkg/apis/machine/types.go
@@ -187,6 +187,10 @@ type LastOperation struct {
 	// Description of the current operation
 	Description string
 
+	// ErrorCode of the current operation if any
+	// +optional
+	ErrorCode string
+
 	// Last update time of current operation
 	LastUpdateTime metav1.Time
 

diff --git a/pkg/apis/machine/v1alpha1/machine_types.go b/pkg/apis/machine/v1alpha1/machine_types.go
@@ -122,6 +122,10 @@ type LastOperation struct {
 	// Description of the current operation
 	Description string `json:"description,omitempty"`
 
+	// ErrorCode of the current operation if any
+	// +optional
+	ErrorCode string `json:"errorCode,omitempty"`
+
 	// Last update time of current operation
 	LastUpdateTime metav1.Time `json:"lastUpdateTime,omitempty"`
 

diff --git a/pkg/apis/machine/v1alpha1/zz_generated.conversion.go b/pkg/apis/machine/v1alpha1/zz_generated.conversion.go
diff --git a/pkg/openapi/openapi_generated.go b/pkg/openapi/openapi_generated.go
diff --git a/pkg/util/provider/machinecodes/status/status.go b/pkg/util/provider/machinecodes/status/status.go
@@ -32,9 +32,6 @@ package status
 
 import (
 	"fmt"
-	"regexp"
-	"strings"
-
 	"github.com/gardener/machine-controller-manager/pkg/util/provider/machinecodes/codes"
 )
 
@@ -105,7 +102,7 @@ func FromError(err error) (s *Status, ok bool) {
 		return nil, true
 	}
 
-	if matches, errInFind := findInString(err.Error()); errInFind == nil {
+	if matches, errInFind := findCodeAndMessage(err.Error()); errInFind == nil {
 		code := codes.StringToCode(matches[0])
 		return &Status{
 			code:    int32(code),
@@ -120,21 +117,35 @@ func FromError(err error) (s *Status, ok bool) {
 	}, false
 }
 
-// findInString need to check if this logic can be optimized
-func findInString(input string) ([]string, error) {
-	var matches []string
-
-	re := regexp.MustCompile(`\[([^\[\]]*)\]`)
-	submatchall := re.FindAllString(input, -1)
-	if submatchall == nil || len(submatchall) != 2 {
-		return nil, fmt.Errorf("Unable to decode for machine code error")
+func findCodeAndMessage(encodedMsg string) ([]string, error) {
+	var decoded []string
+	var temp []rune
+	counter := 0
+
+	for _, char := range encodedMsg {
+		switch char {
+		case '[':
+			counter++
+			temp = append(temp, char)
+		case ']':
+			if counter > 0 {
+				counter--
+				temp = append(temp, char)
+				if counter == 0 {
+					tempString := string(temp)
+					decoded = append(decoded, tempString[1:len(tempString)-1])
+					temp = nil
+				}
+			}
+		default:
+			if counter > 0 {
+				temp = append(temp, char)
+			}
+		}
 	}
 
-	for _, element := range submatchall {
-		element = strings.Trim(element, "[")
-		element = strings.Trim(element, "]")
-		matches = append(matches, element)
+	if len(decoded) != 2 {
+		return nil, fmt.Errorf("unable to decode for machine code error")
 	}
-
-	return matches, nil
+	return decoded, nil
 }
diff --git a/pkg/util/provider/machinecontroller/machine_test.go b/pkg/util/provider/machinecontroller/machine_test.go
@@ -503,6 +503,11 @@ var _ = Describe("machine", func() {
 				} else {
 					Expect(actual.Labels).To(Equal(data.expect.machine.Labels))
 				}
+				if data.expect.machine.Status.LastOperation.ErrorCode != "" {
+					Expect(actual.Status.LastOperation.ErrorCode).To(Equal(data.expect.machine.Status.LastOperation.ErrorCode))
+				} else {
+					Expect(actual.Status.LastOperation.ErrorCode).To(Equal(""))
+				}
 			},
 
 			Entry("Machine creation succeeds with object UPDATE", &data{
@@ -728,7 +733,7 @@ var _ = Describe("machine", func() {
 					retry: machineutils.LongRetry,
 				},
 			}),
-			Entry("Machine creation fails with CrashLoopBackOff", &data{
+			Entry("Machine creation fails with CrashLoopBackOff due to Internal error", &data{
 				setup: setup{
 					secrets: []*corev1.Secret{
 						{
@@ -755,10 +760,8 @@ var _ = Describe("machine", func() {
 				action: action{
 					machine: "machine-0",
 					fakeDriver: &driver.FakeDriver{
-						VMExists:   false,
-						ProviderID: "fakeID-0",
-						NodeName:   "fakeNode-0",
-						Err:        status.Error(codes.Internal, "Provider is returning error on create call"),
+						VMExists: false,
+						Err:      status.Error(codes.Internal, "Provider is returning error on create call"),
 					},
 				},
 				expect: expect{
@@ -774,6 +777,61 @@ var _ = Describe("machine", func() {
 						CurrentStatus: v1alpha1.CurrentStatus{
 							Phase: v1alpha1.MachineCrashLoopBackOff,
 						},
+						LastOperation: v1alpha1.LastOperation{
+							ErrorCode: codes.Internal.String(),
+						},
+					}, nil, nil, nil, true, metav1.Now()),
+					err:   nil,
+					retry: machineutils.MediumRetry,
+				},
+			}),
+			Entry("Machine creation fails with CrashLoopBackOff due to resource exhaustion", &data{
+				setup: setup{
+					secrets: []*corev1.Secret{
+						{
+							ObjectMeta: *newObjectMeta(objMeta, 0),
+							Data:       map[string][]byte{"userData": []byte("test")},
+						},
+					},
+					machineClasses: []*v1alpha1.MachineClass{
+						{
+							ObjectMeta: *newObjectMeta(objMeta, 0),
+							SecretRef:  newSecretReference(objMeta, 0),
+						},
+					},
+					machines: newMachines(1, &v1alpha1.MachineTemplateSpec{
+						ObjectMeta: *newObjectMeta(objMeta, 0),
+						Spec: v1alpha1.MachineSpec{
+							Class: v1alpha1.ClassSpec{
+								Kind: "MachineClass",
+								Name: "machine-0",
+							},
+						},
+					}, nil, nil, nil, nil, true, metav1.Now()),
+				},
+				action: action{
+					machine: "machine-0",
+					fakeDriver: &driver.FakeDriver{
+						VMExists: false,
+						Err:      status.Error(codes.ResourceExhausted, "Provider does not have capacity to create VM"),
+					},
+				},
+				expect: expect{
+					machine: newMachine(&v1alpha1.MachineTemplateSpec{
+						ObjectMeta: *newObjectMeta(objMeta, 0),
+						Spec: v1alpha1.MachineSpec{
+							Class: v1alpha1.ClassSpec{
+								Kind: "MachineClass",
+								Name: "machineClass",
+							},
+						},
+					}, &v1alpha1.MachineStatus{
+						CurrentStatus: v1alpha1.CurrentStatus{
+							Phase: v1alpha1.MachineCrashLoopBackOff,
+						},
+						LastOperation: v1alpha1.LastOperation{
+							ErrorCode: codes.ResourceExhausted.String(),
+						},
 					}, nil, nil, nil, true, metav1.Now()),
 					err:   nil,
 					retry: machineutils.MediumRetry,
@@ -806,10 +864,8 @@ var _ = Describe("machine", func() {
 				action: action{
 					machine: "machine-0",
 					fakeDriver: &driver.FakeDriver{
-						VMExists:   false,
-						ProviderID: "fakeID-0",
-						NodeName:   "fakeNode-0",
-						Err:        status.Error(codes.Internal, "Provider is returning error on create call"),
+						VMExists: false,
+						Err:      status.Error(codes.Internal, "Provider is returning error on create call"),
 					},
 				},
 				expect: expect{
@@ -825,6 +881,9 @@ var _ = Describe("machine", func() {
 						CurrentStatus: v1alpha1.CurrentStatus{
 							Phase: v1alpha1.MachineFailed,
 						},
+						LastOperation: v1alpha1.LastOperation{
+							ErrorCode: codes.Internal.String(),
+						},
 					}, nil, nil, nil, true, metav1.NewTime(metav1.Now().Add(-time.Hour))),
 					err:   nil,
 					retry: machineutils.MediumRetry,

diff --git a/pkg/util/provider/machinecontroller/machine_util.go b/pkg/util/provider/machinecontroller/machine_util.go
@@ -479,7 +479,8 @@ func (c *controller) machineCreateErrorHandler(ctx context.Context, machine *v1a
 		retryRequired  = machineutils.MediumRetry
 		lastKnownState string
 	)
-	if machineErr, ok := status.FromError(err); ok {
+	machineErr, ok := status.FromError(err)
+	if ok {
 		switch machineErr.Code() {
 		case codes.Unknown, codes.DeadlineExceeded, codes.Aborted, codes.Unavailable:
 			retryRequired = machineutils.ShortRetry
@@ -495,6 +496,7 @@ func (c *controller) machineCreateErrorHandler(ctx context.Context, machine *v1a
 		machine,
 		v1alpha1.LastOperation{
 			Description:    "Cloud provider message - " + err.Error(),
+			ErrorCode:      machineErr.Code().String(),
 			State:          v1alpha1.MachineStateFailed,
 			Type:           v1alpha1.MachineOperationCreate,
 			LastUpdateTime: metav1.Now(),