Maintenance Events resource was transfered to the APIv2 #555

testisnullus · 2023-09-13T10:29:37Z

Blocked until the https://instaclustr.atlassian.net/browse/INS-25547 issue is resolved

ribaraka · 2023-09-15T07:17:07Z

apis/clusterresources/v1beta1/maintenanceevents_types.go

 }

 // MaintenanceEventsStatus defines the observed state of MaintenanceEvents
 type MaintenanceEventsStatus struct {
-	EventsStatuses []*MaintenanceEventStatus `json:"eventsStatuses,omitempty"`
+	RescheduledEvent MaintenanceEventReschedule `json:"rescheduled"`


RescheduledEvent -> CurrentRescheduledEvent

ribaraka · 2023-09-15T07:24:23Z

apis/clusterresources/v1beta1/maintenanceevents_types.go

+
+type MaintenanceEventReschedule struct {
+	ScheduledStartTime string `json:"scheduledStartTime"`
+	MaintenanceEventId string `json:"maintenanceEventId"`


MaintenanceEventId -> MaintenanceEventID

ribaraka · 2023-09-15T07:32:39Z

apis/clusterresources/v1beta1/maintenanceevents_webhook.go

+	for _, event := range r.Spec.MaintenanceEventsReschedules {
+		if dateValid, err := validation.ValidateISODate(event.ScheduledStartTime); err != nil || !dateValid {
+			return fmt.Errorf("scheduledStartTime must be provided in an ISO-8601 formatted UTC string: %v", err)
+		}
+	}
+


the last part of the 79 line:

; err != nil || !dateValid

so the validation.ValidateISODate function may not have an error, and still dateValid variable may not be valid?

If the customer provides scheduledStartTime in an invalid format, we will return the error message

why then check for !dateValid, if there is always an error returned by invalid format?

ribaraka · 2023-09-15T07:52:58Z

apis/clusters/v1beta1/structs.go

 	return true
 }

+func areClusteredMaintenanceEventStatusEqual(a, b *clusterresource.MaintenanceEventStatus) bool {
+	return a.ID == b.ID &&


a.ID == b.ID shouldn't be used in comparing, instead, use this condition to actually be sure that this is the same maintenance event and after only perform equality func.

Good catch, will fix

ribaraka · 2023-09-15T07:53:48Z

apis/clusters/v1beta1/structs.go

@@ -509,6 +542,28 @@ func (c *Cluster) CloudProviderSettingsFromInstAPI(iDC models.DataCentre) (setti
 	return
 }

+func (c *Cluster) CloudProviderSettingsFromInstAPIv1(iProviders []*models.ClusterProviderV1) (accountName string, settings []*CloudProviderSettings) {


Shouldn't this be removed?

Removed, it seems that I've accidentally added this method when resolving conflicts

ribaraka · 2023-09-15T07:54:30Z

config/samples/clusters_v1beta1_cassandra.yaml

@@ -3,7 +3,7 @@ kind: Cassandra
 metadata:
  name: cassandra-cluster
 spec:
-  name: "username-Cassandra"
+  name: "danylo-Cassandra"


leave it as it was

ribaraka · 2023-09-15T08:03:10Z

controllers/clusterresources/maintenanceevents_controller.go

+		patch := me.NewPatch()
+		me.Status.RescheduledEvent = *meReschedule
+		err = r.Status().Patch(ctx, me, patch)
 		if err != nil {
-			l.Error(err, "Cannot get Maintenance Events resource",
-				"resource", me,
+			l.Error(err,
+				"Cannot patch Maintenance Event status",
+				"Maintenance Event ID", me.Status.RescheduledEvent.MaintenanceEventId,
 			)
-
-			return err
+			r.EventRecorder.Eventf(
+				me, models.Warning, models.PatchFailed,
+				"Resource status patch is failed. Reason: %v",
+				err,
+			)
+			return models.ReconcileRequeue, nil
 		}

-		var updated bool
-		patch := me.NewPatch()
-		instMEventsStatuses, err := r.API.GetMaintenanceEventsStatuses(me.Spec.ClusterID)
+		patch = me.NewPatch()
+		me.Status.RescheduledEvent.MaintenanceEventId = ""
+		me.Status.RescheduledEvent.ScheduledStartTime = ""
+		err = r.Status().Patch(ctx, me, patch)


There is no point in filling out the Status and then removing it.

Valid point, will fix

ribaraka · 2023-09-15T08:06:32Z

controllers/clusters/cadence_controller.go

+	if !c.Status.AreMaintenanceEventStatusesEqual(iMEStatuses) {
+		patch := c.NewPatch()
+		c.Status.MaintenanceEvents = iMEStatuses
+		err = r.Status().Patch(context.TODO(), c, patch)


please use ctx from the func param

fixed for all similar cases

ribaraka · 2023-09-15T08:11:17Z

controllers/clusters/helpers.go

 	"encoding/json"
 	"fmt"
+	clusterresourcesv1beta1 "github.com/instaclustr/operator/apis/clusterresources/v1beta1"
+	"github.com/instaclustr/operator/pkg/instaclustr"
 	"sort"


busted 🐾 🐾 🐾

ribaraka · 2023-09-15T08:43:06Z

controllers/clusters/helpers.go

+func fetchMaintenanceEventStatuses(
+	api instaclustr.API,
+	clusterID string,
+) ([]*clusterresourcesv1beta1.ClusteredMaintenanceEventStatus,
+	error) {
+


How about making it as a method for instaclustr.API client?

That sounds pretty logical, will move to the client

worryg0d · 2023-09-15T08:49:04Z

apis/clusterresources/v1beta1/maintenanceevents_types.go

+type ClusteredMaintenanceEventStatus struct {
+	InProgress []*MaintenanceEventStatus `json:"inProgress"`
+	Past       []*MaintenanceEventStatus `json:"past"`
+	Upcoming   []*MaintenanceEventStatus `json:"Upcoming"`


Suggested change

Upcoming []*MaintenanceEventStatus `json:"Upcoming"`

Upcoming []*MaintenanceEventStatus `json:"upcoming"`

worryg0d · 2023-09-15T08:57:16Z

controllers/clusters/cassandra_controller.go

+	if !c.Status.AreMaintenanceEventStatusesEqual(iMEStatuses) {
+		patch := c.NewPatch()
+		c.Status.MaintenanceEvents = iMEStatuses
+		err = r.Status().Patch(context.TODO(), c, patch)


Please use the provided ctx from the func params. Please do it in all such cases

testisnullus added the refactor Code improvements or refactorings label Sep 13, 2023

testisnullus requested review from RostislavPorohnya, ribaraka and worryg0d September 13, 2023 10:29

testisnullus self-assigned this Sep 13, 2023

testisnullus changed the title ~~Maintenance Events resource was transfered to APIv2~~ [WIP] Maintenance Events resource was transfered to APIv2 Sep 13, 2023

testisnullus force-pushed the maintenance-events-to-apiv2 branch 3 times, most recently from 7f8e3d1 to 518b189 Compare September 13, 2023 10:52

testisnullus changed the title ~~[WIP] Maintenance Events resource was transfered to APIv2~~ [WIP] Maintenance Events resource was transfered to the APIv2 Sep 13, 2023

testisnullus added the blocked Some events are blocking the issue or pull request label Sep 13, 2023

testisnullus force-pushed the maintenance-events-to-apiv2 branch from 518b189 to 2cc9c6c Compare September 14, 2023 16:33

testisnullus changed the title ~~[WIP] Maintenance Events resource was transfered to the APIv2~~ Maintenance Events resource was transfered to the APIv2 Sep 14, 2023

testisnullus removed the blocked Some events are blocking the issue or pull request label Sep 14, 2023

testisnullus force-pushed the maintenance-events-to-apiv2 branch from 2cc9c6c to 4400f48 Compare September 14, 2023 16:36

ribaraka requested changes Sep 15, 2023

View reviewed changes

ribaraka reviewed Sep 15, 2023

View reviewed changes

worryg0d reviewed Sep 15, 2023

View reviewed changes

testisnullus force-pushed the maintenance-events-to-apiv2 branch 2 times, most recently from bcce9bb to b3f2910 Compare September 15, 2023 10:13

testisnullus requested review from ribaraka and worryg0d September 15, 2023 10:13

testisnullus force-pushed the maintenance-events-to-apiv2 branch from b3f2910 to d2ec9ae Compare September 15, 2023 10:19

worryg0d approved these changes Sep 15, 2023

View reviewed changes

OleksiienkoMykyta approved these changes Sep 15, 2023

View reviewed changes

testisnullus force-pushed the maintenance-events-to-apiv2 branch from d2ec9ae to b42cb94 Compare September 15, 2023 11:32

ribaraka approved these changes Sep 15, 2023

View reviewed changes

Maintenance Events resource was transfered to APIv2

750f126

testisnullus force-pushed the maintenance-events-to-apiv2 branch from b42cb94 to 750f126 Compare September 15, 2023 12:19

testisnullus merged commit e5603dd into main Sep 15, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Maintenance Events resource was transfered to the APIv2 #555

Maintenance Events resource was transfered to the APIv2 #555

testisnullus commented Sep 13, 2023 •

edited

Loading

ribaraka Sep 15, 2023

testisnullus Sep 15, 2023

ribaraka Sep 15, 2023

testisnullus Sep 15, 2023

ribaraka Sep 15, 2023

testisnullus Sep 15, 2023

ribaraka Sep 15, 2023

ribaraka Sep 15, 2023

testisnullus Sep 15, 2023

ribaraka Sep 15, 2023

testisnullus Sep 15, 2023

ribaraka Sep 15, 2023

testisnullus Sep 15, 2023

ribaraka Sep 15, 2023

testisnullus Sep 15, 2023

ribaraka Sep 15, 2023

testisnullus Sep 15, 2023

ribaraka Sep 15, 2023

testisnullus Sep 15, 2023

ribaraka Sep 15, 2023

testisnullus Sep 15, 2023

worryg0d Sep 15, 2023

testisnullus Sep 15, 2023

worryg0d Sep 15, 2023

testisnullus Sep 15, 2023

	Upcoming []*MaintenanceEventStatus `json:"Upcoming"`
	Upcoming []*MaintenanceEventStatus `json:"upcoming"`

Maintenance Events resource was transfered to the APIv2 #555

Maintenance Events resource was transfered to the APIv2 #555

Conversation

testisnullus commented Sep 13, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

testisnullus commented Sep 13, 2023 •

edited

Loading