Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Labels sync targets with the SyncTargetKey for reverse lookup #1672

Merged
merged 4 commits into from
Aug 4, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions config/crds/workload.kcp.dev_synctargets.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ spec:
name: Synced API resources
priority: 3
type: string
- jsonPath: .metadata.labels['internal\.workload\.kcp\.dev/key']
name: Key
priority: 4
type: string
- jsonPath: .metadata.creationTimestamp
name: Age
type: date
Expand Down
2 changes: 1 addition & 1 deletion config/root-phase0/apiexport-workload.kcp.dev.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,5 @@ metadata:
name: workload.kcp.dev
spec:
latestResourceSchemas:
- v220801-c65c674d4.synctargets.workload.kcp.dev
- v220803-727d944d.synctargets.workload.kcp.dev
status: {}
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ apiVersion: apis.kcp.dev/v1alpha1
kind: APIResourceSchema
metadata:
creationTimestamp: null
name: v220801-c65c674d4.synctargets.workload.kcp.dev
name: v220803-727d944d.synctargets.workload.kcp.dev
spec:
group: workload.kcp.dev
names:
Expand All @@ -27,6 +27,10 @@ spec:
name: Synced API resources
priority: 3
type: string
- jsonPath: .metadata.labels['internal\.workload\.kcp\.dev/key']
name: Key
priority: 4
type: string
- jsonPath: .metadata.creationTimestamp
name: Age
type: date
Expand Down
1 change: 1 addition & 0 deletions pkg/apis/workload/v1alpha1/synctarget_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import (
// +kubebuilder:printcolumn:name="Location",type="string",JSONPath=`.metadata.name`,priority=1
// +kubebuilder:printcolumn:name="Ready",type="string",JSONPath=`.status.conditions[?(@.type=="Ready")].status`,priority=2
// +kubebuilder:printcolumn:name="Synced API resources",type="string",JSONPath=`.status.syncedResources`,priority=3
// +kubebuilder:printcolumn:name="Key",type="string",JSONPath=`.metadata.labels['internal\.workload\.kcp\.dev/key']`,priority=4
// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp"
type SyncTarget struct {
metav1.TypeMeta `json:",inline"`
Expand Down
4 changes: 4 additions & 0 deletions pkg/apis/workload/v1alpha1/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,4 +116,8 @@ const (
// InternalSyncTargetPlacementAnnotationKey is a internal annotation key on placement API to mark the synctarget scheduled
// from this placement. The value is a hash of the SyncTarget workspace + SyncTarget name, generated with the ToSyncTargetKey(..) helper func.
InternalSyncTargetPlacementAnnotationKey = "internal.workload.kcp.dev/synctarget"

// InternalSyncTargetKeyLabel is an internal label set on a SyncTarget resource that contains the full hash of the SyncTargetKey, generated with the ToSyncTargetKey(..)
// helper func, this label is used for reverse lookups of a syncTargetKey to SyncTarget.
InternalSyncTargetKeyLabel = "internal.workload.kcp.dev/key"
)
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package virtualworkspaceurls
package synctarget

import (
"context"
Expand Down Expand Up @@ -191,11 +191,21 @@ func (c *Controller) process(ctx context.Context, key string) error {
return err
}

if _, err := c.kcpClusterClient.WorkloadV1alpha1().SyncTargets().Patch(logicalcluster.WithCluster(ctx, logicalcluster.From(currentSyncTarget)), currentSyncTarget.Name, types.MergePatchType, patchBytes, metav1.PatchOptions{}, "status"); err != nil {
klog.Errorf("failed to patch sync target status: %v", err)
return err
if !reflect.DeepEqual(currentSyncTarget.ObjectMeta, newSyncTarget.ObjectMeta) || !reflect.DeepEqual(currentSyncTarget.Spec, newSyncTarget.Spec) {
klog.V(2).InfoS("patching synctarget", "name", newSyncTarget.Name, "workspace", logicalcluster.From(newSyncTarget), "patchbytes", string(patchBytes))
if _, err := c.kcpClusterClient.WorkloadV1alpha1().SyncTargets().Patch(logicalcluster.WithCluster(ctx, logicalcluster.From(currentSyncTarget)), currentSyncTarget.Name, types.MergePatchType, patchBytes, metav1.PatchOptions{}); err != nil {
klog.Errorf("failed to patch sync target: %v", err)
return err
}
}

if !reflect.DeepEqual(currentSyncTarget.Status, newSyncTarget.Status) {
klog.V(2).InfoS("patching synctarget status", "name", newSyncTarget.Name, "workspace", logicalcluster.From(newSyncTarget), "patchbytes", string(patchBytes))
if _, err := c.kcpClusterClient.WorkloadV1alpha1().SyncTargets().Patch(logicalcluster.WithCluster(ctx, logicalcluster.From(currentSyncTarget)), currentSyncTarget.Name, types.MergePatchType, patchBytes, metav1.PatchOptions{}, "status"); err != nil {
klog.Errorf("failed to patch sync target status: %v", err)
return err
}
}
klog.V(2).InfoS("updated sync target status", "SyncTarget", newSyncTarget.Name, "LogicalCluster", logicalcluster.From(newSyncTarget))

return nil
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package virtualworkspaceurls
package synctarget

import (
"net/url"
Expand All @@ -34,6 +34,13 @@ import (
func (c *Controller) reconcile(syncTarget *workloadv1alpha1.SyncTarget, workspaceShards []*v1alpha1.ClusterWorkspaceShard) (*workloadv1alpha1.SyncTarget, error) {
syncTargetCopy := syncTarget.DeepCopy()

labels := syncTargetCopy.GetLabels()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can edit syncTargetCopy.Labels directly. No need for Get/SetLabel.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that panics.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no. Of course you have to create the map if it is nil. But no need for calling setter and getter.

if labels == nil {
labels = map[string]string{}
}
labels[workloadv1alpha1.InternalSyncTargetKeyLabel] = workloadv1alpha1.ToSyncTargetKey(logicalcluster.From(syncTargetCopy), syncTargetCopy.Name)
syncTargetCopy.SetLabels(labels)

desiredURLs := sets.NewString()
for _, workspaceShard := range workspaceShards {
if workspaceShard.Spec.ExternalURL != "" {
Expand All @@ -53,6 +60,17 @@ func (c *Controller) reconcile(syncTarget *workloadv1alpha1.SyncTarget, workspac
}
}

if syncTargetCopy.Status.VirtualWorkspaces != nil {
currentURLs := sets.NewString()
for _, virtualWorkspace := range syncTargetCopy.Status.VirtualWorkspaces {
currentURLs = currentURLs.Insert(virtualWorkspace.URL)
}

if desiredURLs.Equal(currentURLs) {
return syncTargetCopy, nil
}
}

syncTargetCopy.Status.VirtualWorkspaces = nil
for _, url := range desiredURLs.List() {
syncTargetCopy.Status.VirtualWorkspaces = append(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package virtualworkspaceurls
package synctarget

import (
"reflect"
Expand Down Expand Up @@ -69,6 +69,9 @@ func TestReconciler(t *testing.T) {
Annotations: map[string]string{
logicalcluster.AnnotationKey: "demo:root:yourworkspace",
},
Labels: map[string]string{
"internal.workload.kcp.dev/key": "2Fhhz9cq06pipXqhKzp8wrxSgTVTUzc8fKKqLI",
},
},
Spec: workloadv1alpha1.SyncTargetSpec{
Unschedulable: false,
Expand Down Expand Up @@ -135,6 +138,9 @@ func TestReconciler(t *testing.T) {
Annotations: map[string]string{
logicalcluster.AnnotationKey: "demo:root:yourworkspace",
},
Labels: map[string]string{
"internal.workload.kcp.dev/key": "2Fhhz9cq06pipXqhKzp8wrxSgTVTUzc8fKKqLI",
},
},
Spec: workloadv1alpha1.SyncTargetSpec{
Unschedulable: false,
Expand Down Expand Up @@ -207,6 +213,9 @@ func TestReconciler(t *testing.T) {
Annotations: map[string]string{
logicalcluster.AnnotationKey: "demo:root:yourworkspace",
},
Labels: map[string]string{
"internal.workload.kcp.dev/key": "2Fhhz9cq06pipXqhKzp8wrxSgTVTUzc8fKKqLI",
},
},
Spec: workloadv1alpha1.SyncTargetSpec{
Unschedulable: false,
Expand Down Expand Up @@ -239,16 +248,17 @@ func TestReconciler(t *testing.T) {
Unschedulable: false,
EvictAfter: nil,
},
Status: workloadv1alpha1.SyncTargetStatus{
VirtualWorkspaces: []workloadv1alpha1.VirtualWorkspace{},
},
Status: workloadv1alpha1.SyncTargetStatus{},
},
expectedSyncTarget: &workloadv1alpha1.SyncTarget{
ObjectMeta: metav1.ObjectMeta{
Name: "test-cluster",
Annotations: map[string]string{
logicalcluster.AnnotationKey: "demo:root:yourworkspace",
},
Labels: map[string]string{
"internal.workload.kcp.dev/key": "2Fhhz9cq06pipXqhKzp8wrxSgTVTUzc8fKKqLI",
},
},
Spec: workloadv1alpha1.SyncTargetSpec{
Unschedulable: false,
Expand Down Expand Up @@ -301,6 +311,9 @@ func TestReconciler(t *testing.T) {
Annotations: map[string]string{
logicalcluster.AnnotationKey: "demo:root:yourworkspace",
},
Labels: map[string]string{
"internal.workload.kcp.dev/key": "2Fhhz9cq06pipXqhKzp8wrxSgTVTUzc8fKKqLI",
},
},
Spec: workloadv1alpha1.SyncTargetSpec{
Unschedulable: false,
Expand Down
8 changes: 4 additions & 4 deletions pkg/server/controllers.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ import (
workloadnamespace "github.com/kcp-dev/kcp/pkg/reconciler/workload/namespace"
workloadplacement "github.com/kcp-dev/kcp/pkg/reconciler/workload/placement"
workloadresource "github.com/kcp-dev/kcp/pkg/reconciler/workload/resource"
virtualworkspaceurlscontroller "github.com/kcp-dev/kcp/pkg/reconciler/workload/virtualworkspaceurls"
synctargetcontroller "github.com/kcp-dev/kcp/pkg/reconciler/workload/synctarget"
)

func (s *Server) installClusterRoleAggregationController(ctx context.Context, config *rest.Config) error {
Expand Down Expand Up @@ -894,15 +894,15 @@ func (s *Server) installWorkloadsAPIExportCreateController(ctx context.Context,
})
}

func (s *Server) installVirtualWorkspaceURLsController(ctx context.Context, config *rest.Config, server *genericapiserver.GenericAPIServer) error {
controllerName := "kcp-virtualworkspace-urls-controller"
func (s *Server) installSyncTargetController(ctx context.Context, config *rest.Config, server *genericapiserver.GenericAPIServer) error {
controllerName := "kcp-synctarget-controller"
config = kcpclienthelper.NewClusterConfig(rest.AddUserAgent(rest.CopyConfig(config), controllerName))
kcpClusterClient, err := kcpclient.NewForConfig(config)
if err != nil {
return err
}

c := virtualworkspaceurlscontroller.NewController(
c := synctargetcontroller.NewController(
kcpClusterClient,
s.KcpSharedInformerFactory.Workload().V1alpha1().SyncTargets(),
s.KcpSharedInformerFactory.Tenancy().V1alpha1().ClusterWorkspaceShards(),
Expand Down
2 changes: 1 addition & 1 deletion pkg/server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,7 @@ func (s *Server) Run(ctx context.Context) error {
if err := s.installSyncTargetHeartbeatController(ctx, controllerConfig); err != nil {
return err
}
if err := s.installVirtualWorkspaceURLsController(ctx, controllerConfig, delegationChainHead); err != nil {
if err := s.installSyncTargetController(ctx, controllerConfig, delegationChainHead); err != nil {
return err
}
}
Expand Down