diff --git a/.golangci.yml b/.golangci.yml index d780e298..ad42a14a 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -1,40 +1,45 @@ +version: "2" run: - timeout: 5m allow-parallel-runners: true - -issues: - # don't skip warning about doc comments - # don't exclude the default set of lint - exclude-use-default: false - # restore some of the defaults - # (fill in the rest as needed) - exclude-rules: - - path: "api/*" - linters: - - lll - - path: "internal/*" - linters: - - dupl - - lll linters: - disable-all: true + default: none enable: + - copyloopvar - dupl - errcheck - - copyloopvar - ginkgolinter - goconst - gocyclo - - gofmt - - goimports - - gosimple - govet - ineffassign - lll - misspell - nakedret - staticcheck - - typecheck - unconvert - unparam - unused + exclusions: + generated: lax + rules: + - linters: + - lll + path: api/* + - linters: + - dupl + - lll + path: internal/* + paths: + - third_party$ + - builtin$ + - examples$ +formatters: + enable: + - gofmt + - goimports + exclusions: + generated: lax + paths: + - third_party$ + - builtin$ + - examples$ diff --git a/Makefile b/Makefile index 68c82bb8..2110f918 100644 --- a/Makefile +++ b/Makefile @@ -180,7 +180,7 @@ GRPCURL = $(LOCALBIN)/grpcurl KUSTOMIZE_VERSION ?= v5.4.1 CONTROLLER_TOOLS_VERSION ?= v0.16.3 ENVTEST_VERSION ?= release-0.18 -GOLANGCI_LINT_VERSION ?= v1.61.0 +GOLANGCI_LINT_VERSION ?= v2.1.2 KIND_VERSION ?= v0.27.0 GRPCURL_VERSION ?= v1.9.2 @@ -202,7 +202,7 @@ $(ENVTEST): $(LOCALBIN) .PHONY: golangci-lint golangci-lint: $(GOLANGCI_LINT) ## Download golangci-lint locally if necessary. $(GOLANGCI_LINT): $(LOCALBIN) - $(call go-install-tool,$(GOLANGCI_LINT),github.com/golangci/golangci-lint/cmd/golangci-lint,$(GOLANGCI_LINT_VERSION)) + $(call go-install-tool,$(GOLANGCI_LINT),github.com/golangci/golangci-lint/v2/cmd/golangci-lint,$(GOLANGCI_LINT_VERSION)) .PHONY: grpcurl grpcurl: $(GRPCURL) ## Download grpcurl locally if necessary. diff --git a/api/v1alpha1/exporter_types.go b/api/v1alpha1/exporter_types.go index 662550f6..9365a5b8 100644 --- a/api/v1alpha1/exporter_types.go +++ b/api/v1alpha1/exporter_types.go @@ -37,6 +37,7 @@ type ExporterStatus struct { Credential *corev1.LocalObjectReference `json:"credential,omitempty"` Devices []Device `json:"devices,omitempty"` LeaseRef *corev1.LocalObjectReference `json:"leaseRef,omitempty"` + LastSeen metav1.Time `json:"lastSeen,omitempty"` Endpoint string `json:"endpoint,omitempty"` } diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 847691b9..b1b0c5a5 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -388,6 +388,7 @@ func (in *ExporterStatus) DeepCopyInto(out *ExporterStatus) { *out = new(v1.LocalObjectReference) **out = **in } + in.LastSeen.DeepCopyInto(&out.LastSeen) } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ExporterStatus. diff --git a/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_exporters.yaml b/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_exporters.yaml index 5c0d0c66..6383bbf9 100644 --- a/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_exporters.yaml +++ b/deploy/helm/jumpstarter/charts/jumpstarter-controller/templates/crds/jumpstarter.dev_exporters.yaml @@ -135,6 +135,9 @@ spec: type: array endpoint: type: string + lastSeen: + format: date-time + type: string leaseRef: description: |- LocalObjectReference contains enough information to let you locate the diff --git a/internal/controller/client_controller.go b/internal/controller/client_controller.go index 7dc8c4c7..d1e7a564 100644 --- a/internal/controller/client_controller.go +++ b/internal/controller/client_controller.go @@ -23,7 +23,6 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" kclient "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" @@ -33,7 +32,7 @@ import ( // ClientReconciler reconciles a Client object type ClientReconciler struct { - client.Client + kclient.Client Scheme *runtime.Scheme Signer *oidc.Signer } diff --git a/internal/controller/exporter_controller.go b/internal/controller/exporter_controller.go index 3c2b85f7..5883cf73 100644 --- a/internal/controller/exporter_controller.go +++ b/internal/controller/exporter_controller.go @@ -19,8 +19,11 @@ package controller import ( "context" "fmt" + "time" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -72,6 +75,11 @@ func (r *ExporterReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c return ctrl.Result{}, err } + result, err := r.reconcileStatusConditionsOnline(ctx, &exporter) + if err != nil { + return ctrl.Result{}, err + } + if err := r.reconcileStatusEndpoint(ctx, &exporter); err != nil { return ctrl.Result{}, err } @@ -80,7 +88,7 @@ func (r *ExporterReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c return RequeueConflict(logger, ctrl.Result{}, err) } - return ctrl.Result{}, nil + return result, nil } func (r *ExporterReconciler) reconcileStatusCredential( @@ -144,6 +152,64 @@ func (r *ExporterReconciler) reconcileStatusEndpoint( return nil } +// nolint:unparam +func (r *ExporterReconciler) reconcileStatusConditionsOnline( + _ context.Context, + exporter *jumpstarterdevv1alpha1.Exporter, +) (ctrl.Result, error) { + var requeueAfter time.Duration = 0 + + if exporter.Status.LastSeen.IsZero() { + meta.SetStatusCondition(&exporter.Status.Conditions, metav1.Condition{ + Type: string(jumpstarterdevv1alpha1.ExporterConditionTypeOnline), + Status: metav1.ConditionFalse, + ObservedGeneration: exporter.Generation, + Reason: "Seen", + Message: "Never seen", + }) + // marking the exporter offline, no need to requeue + } else if time.Since(exporter.Status.LastSeen.Time) > time.Minute { + meta.SetStatusCondition(&exporter.Status.Conditions, metav1.Condition{ + Type: string(jumpstarterdevv1alpha1.ExporterConditionTypeOnline), + Status: metav1.ConditionFalse, + ObservedGeneration: exporter.Generation, + Reason: "Seen", + Message: "Last seen more than 1 minute ago", + }) + // marking the exporter offline, no need to requeue + } else { + meta.SetStatusCondition(&exporter.Status.Conditions, metav1.Condition{ + Type: string(jumpstarterdevv1alpha1.ExporterConditionTypeOnline), + Status: metav1.ConditionTrue, + ObservedGeneration: exporter.Generation, + Reason: "Seen", + Message: "Lase seen less than 1 minute ago", + }) + // marking the exporter online, requeue after 30 seconds + requeueAfter = time.Second * 30 + } + + if exporter.Status.Devices == nil { + meta.SetStatusCondition(&exporter.Status.Conditions, metav1.Condition{ + Type: string(jumpstarterdevv1alpha1.ExporterConditionTypeRegistered), + Status: metav1.ConditionFalse, + ObservedGeneration: exporter.Generation, + Reason: "Unregister", + }) + } else { + meta.SetStatusCondition(&exporter.Status.Conditions, metav1.Condition{ + Type: string(jumpstarterdevv1alpha1.ExporterConditionTypeRegistered), + Status: metav1.ConditionTrue, + ObservedGeneration: exporter.Generation, + Reason: "Register", + }) + } + + return ctrl.Result{ + RequeueAfter: requeueAfter, + }, nil +} + // SetupWithManager sets up the controller with the Manager. func (r *ExporterReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). diff --git a/internal/controller/lease_controller.go b/internal/controller/lease_controller.go index afe40118..bcb548fc 100644 --- a/internal/controller/lease_controller.go +++ b/internal/controller/lease_controller.go @@ -465,15 +465,13 @@ func filterOutOfflineExporters(matchingExporters []jumpstarterdevv1alpha1.Export onlineExporters := slices.DeleteFunc( matchingExporters, func(exporter jumpstarterdevv1alpha1.Exporter) bool { - return !(true && - meta.IsStatusConditionTrue( - exporter.Status.Conditions, - string(jumpstarterdevv1alpha1.ExporterConditionTypeRegistered), - ) && - meta.IsStatusConditionTrue( - exporter.Status.Conditions, - string(jumpstarterdevv1alpha1.ExporterConditionTypeOnline), - )) + return !true || !meta.IsStatusConditionTrue( + exporter.Status.Conditions, + string(jumpstarterdevv1alpha1.ExporterConditionTypeRegistered), + ) || !meta.IsStatusConditionTrue( + exporter.Status.Conditions, + string(jumpstarterdevv1alpha1.ExporterConditionTypeOnline), + ) }, ) return onlineExporters diff --git a/internal/controller/lease_controller_test.go b/internal/controller/lease_controller_test.go index 4cd12c75..9421fcbe 100644 --- a/internal/controller/lease_controller_test.go +++ b/internal/controller/lease_controller_test.go @@ -339,6 +339,13 @@ func setExporterOnlineConditions(ctx context.Context, name string, status metav1 Status: status, Reason: "dummy", }) + if status == metav1.ConditionTrue { + exporter.Status.Devices = []jumpstarterdevv1alpha1.Device{{}} + exporter.Status.LastSeen = metav1.Now() + } else { + exporter.Status.Devices = nil + exporter.Status.LastSeen = metav1.NewTime(metav1.Now().Add(-time.Minute * 2)) + } Expect(k8sClient.Status().Update(ctx, exporter)).To(Succeed()) } diff --git a/internal/controller/secret_helpers.go b/internal/controller/secret_helpers.go index e75519b8..658dbf02 100644 --- a/internal/controller/secret_helpers.go +++ b/internal/controller/secret_helpers.go @@ -6,7 +6,6 @@ import ( "github.com/jumpstarter-dev/jumpstarter-controller/internal/oidc" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" - apisv1 "k8s.io/apimachinery/pkg/apis/meta/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -23,7 +22,7 @@ func ensureSecret( scheme *runtime.Scheme, signer *oidc.Signer, subject string, - owner apisv1.Object, + owner metav1.Object, ) (*corev1.Secret, error) { logger := log.FromContext(ctx).WithName("ensureSecret") var secret corev1.Secret diff --git a/internal/protocol/jumpstarter/v1/jumpstarter.pb.go b/internal/protocol/jumpstarter/v1/jumpstarter.pb.go index d5c0db49..6a2f950b 100644 --- a/internal/protocol/jumpstarter/v1/jumpstarter.pb.go +++ b/internal/protocol/jumpstarter/v1/jumpstarter.pb.go @@ -86,7 +86,7 @@ func (x *RegisterRequest) GetReports() []*DriverInstanceReport { type DriverInstanceReport struct { state protoimpl.MessageState `protogen:"open.v1"` - Uuid string `protobuf:"bytes,1,opt,name=uuid,proto3" json:"uuid,omitempty"` // a unique id within the expoter + Uuid string `protobuf:"bytes,1,opt,name=uuid,proto3" json:"uuid,omitempty"` // a unique id within the exporter ParentUuid *string `protobuf:"bytes,2,opt,name=parent_uuid,json=parentUuid,proto3,oneof" json:"parent_uuid,omitempty"` // optional, if device has a parent device Labels map[string]string `protobuf:"bytes,3,rep,name=labels,proto3" json:"labels,omitempty" protobuf_key:"bytes,1,opt,name=key" protobuf_val:"bytes,2,opt,name=value"` unknownFields protoimpl.UnknownFields diff --git a/internal/service/controller_service.go b/internal/service/controller_service.go index 59ebb654..885be683 100644 --- a/internal/service/controller_service.go +++ b/internal/service/controller_service.go @@ -46,7 +46,6 @@ import ( "google.golang.org/protobuf/types/known/durationpb" "google.golang.org/protobuf/types/known/timestamppb" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/runtime" @@ -62,7 +61,7 @@ import ( "github.com/jumpstarter-dev/jumpstarter-controller/internal/controller" ) -// ControlerService exposes a gRPC service +// ControllerService exposes a gRPC service type ControllerService struct { pb.UnimplementedControllerServiceServer Client client.WithWatch @@ -151,16 +150,6 @@ func (s *ControllerService) Register(ctx context.Context, req *pb.RegisterReques original = client.MergeFrom(exporter.DeepCopy()) - meta.SetStatusCondition(&exporter.Status.Conditions, metav1.Condition{ - Type: string(jumpstarterdevv1alpha1.ExporterConditionTypeRegistered), - Status: metav1.ConditionTrue, - ObservedGeneration: exporter.Generation, - LastTransitionTime: metav1.Time{ - Time: time.Now(), - }, - Reason: "Register", - }) - devices := []jumpstarterdevv1alpha1.Device{} for _, device := range req.Reports { devices = append(devices, jumpstarterdevv1alpha1.Device{ @@ -202,16 +191,7 @@ func (s *ControllerService) Unregister( }) original := client.MergeFrom(exporter.DeepCopy()) - meta.SetStatusCondition(&exporter.Status.Conditions, metav1.Condition{ - Type: string(jumpstarterdevv1alpha1.ExporterConditionTypeRegistered), - Status: metav1.ConditionFalse, - ObservedGeneration: exporter.Generation, - LastTransitionTime: metav1.Time{ - Time: time.Now(), - }, - Reason: "Bye", - Message: req.GetReason(), - }) + exporter.Status.Devices = nil if err := s.Client.Status().Patch(ctx, exporter, original); err != nil { logger.Error(err, "unable to update exporter status") @@ -293,34 +273,6 @@ func (s *ControllerService) Status(req *pb.StatusRequest, stream pb.ControllerSe Name: exporter.Name, }) - defer func() { - logger.Info("Status stream terminating, marking exporter offline") - // Make sure defer runs under a fresh context - // otherwise these operations would fail if the rpc context is cancelled - ctx, cancel := context.WithTimeout(context.Background(), time.Second*10) - if err := s.Client.Get( - ctx, - types.NamespacedName{Name: exporter.Name, Namespace: exporter.Namespace}, - exporter, - ); err != nil { - logger.Error(err, "unable to refresh exporter status, continuing anyway") - } - original := client.MergeFrom(exporter.DeepCopy()) - meta.SetStatusCondition(&exporter.Status.Conditions, metav1.Condition{ - Type: string(jumpstarterdevv1alpha1.ExporterConditionTypeOnline), - Status: metav1.ConditionFalse, - ObservedGeneration: exporter.Generation, - LastTransitionTime: metav1.Time{ - Time: time.Now(), - }, - Reason: "Disconnect", - }) - if err = s.Client.Status().Patch(ctx, exporter, original); err != nil { - logger.Error(err, "unable to update exporter status, continuing anyway") - } - cancel() - }() - watcher, err := s.Client.Watch(ctx, &jumpstarterdevv1alpha1.ExporterList{}, &client.ListOptions{ FieldSelector: fields.OneTermEqualSelector("metadata.name", exporter.Name), Namespace: exporter.Namespace, @@ -332,66 +284,72 @@ func (s *ControllerService) Status(req *pb.StatusRequest, stream pb.ControllerSe defer watcher.Stop() - for result := range watcher.ResultChan() { - switch result.Type { - case watch.Added, watch.Modified, watch.Deleted: - exporter = result.Object.(*jumpstarterdevv1alpha1.Exporter) - leased := exporter.Status.LeaseRef != nil - leaseName := (*string)(nil) - clientName := (*string)(nil) - - original := client.MergeFrom(exporter.DeepCopy()) - if false || - meta.SetStatusCondition(&exporter.Status.Conditions, metav1.Condition{ - Type: string(jumpstarterdevv1alpha1.ExporterConditionTypeOnline), - Status: metav1.ConditionTrue, - ObservedGeneration: exporter.Generation, - Reason: "Connect", - Message: "Exporter is connected to the Status endpoint", - }) || - meta.SetStatusCondition(&exporter.Status.Conditions, metav1.Condition{ - Type: string(jumpstarterdevv1alpha1.ExporterConditionTypeRegistered), - Status: metav1.ConditionTrue, - ObservedGeneration: exporter.Generation, - Reason: "Connect", - Message: "Exporter is connected to the Status endpoint", - }) { - if err = s.Client.Status().Patch(ctx, exporter, original); err != nil { - logger.Error(err, "unable to update exporter status") + ticker := time.NewTicker(time.Second * 10) + + defer ticker.Stop() + + online := func() { + original := client.MergeFrom(exporter.DeepCopy()) + exporter.Status.LastSeen = metav1.Now() + if err = s.Client.Status().Patch(ctx, exporter, original); err != nil { + logger.Error(err, "unable to update exporter status.lastSeen") + } + } + + // ticker does not tick instantly, thus calling online immediately once + // https://github.com/golang/go/issues/17601 + select { + case <-ctx.Done(): + return nil + default: + online() + } + + for { + select { + case <-ctx.Done(): + logger.Info("Status stream terminated normally") + return nil + case <-ticker.C: + online() + case result := <-watcher.ResultChan(): + switch result.Type { + case watch.Added, watch.Modified, watch.Deleted: + exporter = result.Object.(*jumpstarterdevv1alpha1.Exporter) + leased := exporter.Status.LeaseRef != nil + leaseName := (*string)(nil) + clientName := (*string)(nil) + + if leased { + leaseName = &exporter.Status.LeaseRef.Name + var lease jumpstarterdevv1alpha1.Lease + if err := s.Client.Get( + ctx, + types.NamespacedName{Namespace: exporter.Namespace, Name: *leaseName}, + &lease, + ); err != nil { + logger.Error(err, "failed to get lease on exporter") + return err + } + clientName = &lease.Spec.ClientRef.Name } - } - if leased { - leaseName = &exporter.Status.LeaseRef.Name - var lease jumpstarterdevv1alpha1.Lease - if err := s.Client.Get( - ctx, - types.NamespacedName{Namespace: exporter.Namespace, Name: *leaseName}, - &lease, - ); err != nil { - logger.Error(err, "failed to get lease on exporter") + status := pb.StatusResponse{ + Leased: leased, + LeaseName: leaseName, + ClientName: clientName, + } + logger.Info("Sending status update to exporter", "status", fmt.Sprintf("%+v", &status)) + if err = stream.Send(&status); err != nil { + logger.Error(err, "Failed to send status update to exporter") return err } - clientName = &lease.Spec.ClientRef.Name - } - - status := pb.StatusResponse{ - Leased: leased, - LeaseName: leaseName, - ClientName: clientName, - } - logger.Info("Sending status update to exporter", "status", fmt.Sprintf("%+v", &status)) - if err = stream.Send(&status); err != nil { - logger.Error(err, "Failed to send status update to exporter") - return err + case watch.Error: + logger.Error(fmt.Errorf("%+v", result.Object), "Received error when watching exporter") + return fmt.Errorf("received error when watching exporter") } - case watch.Error: - logger.Error(fmt.Errorf("%+v", result.Object), "Received error when watching exporter") - return fmt.Errorf("received error when watching exporter") } } - logger.Info("Status stream terminated normally") - return nil } func (s *ControllerService) Dial(ctx context.Context, req *pb.DialRequest) (*pb.DialResponse, error) { @@ -578,7 +536,7 @@ func (s *ControllerService) RequestLease( return nil, err } - var lease jumpstarterdevv1alpha1.Lease = jumpstarterdevv1alpha1.Lease{ + var lease = jumpstarterdevv1alpha1.Lease{ ObjectMeta: metav1.ObjectMeta{ Namespace: client.Namespace, Name: leaseName.String(), diff --git a/test/utils/utils.go b/test/utils/utils.go index 63413095..0deb50a3 100644 --- a/test/utils/utils.go +++ b/test/utils/utils.go @@ -22,7 +22,7 @@ import ( "os/exec" "strings" - . "github.com/onsi/ginkgo/v2" //nolint:golint,revive + . "github.com/onsi/ginkgo/v2" //nolint:staticcheck ) const ( @@ -135,6 +135,6 @@ func GetProjectDir() (string, error) { if err != nil { return wd, err } - wd = strings.Replace(wd, "/test/e2e", "", -1) + wd = strings.ReplaceAll(wd, "/test/e2e", "") return wd, nil } diff --git a/typos.toml b/typos.toml new file mode 100644 index 00000000..e8741358 --- /dev/null +++ b/typos.toml @@ -0,0 +1,6 @@ +[default.extend-words] +Ded = "Ded" # from ANDed + +[type.gomod] +extend-glob = ["go.mod", "go.sum"] +check-file = false