Add Status subresource to ClusterGroup CRD #1778

abhiraut · 2021-01-23T02:34:51Z

Add status sub resource to ClusterGroup CRD. The status will store the conditions associated with a CG.
Currently, supported values for status.conditions are conditions of type GroupMembersComputed. The condition status of the GroupMembersComputed will store whether the CG has computed its members. Valid values for the condition status of GroupMembersComputed type are "True" and "False". The condition status will be updated by the controller to "True" once group members have been calculated and updated to the corresponding internal Group.

The Status of the ClusterGroup can be retrieved by performing the following GET:

eg: Get status for ClusterGroup "mycg"
GET $K8sAPI/apis/core.antrea.tanzu.vmware.com/v1alpha2/clustergroups/mycg

{
   "apiVersion":"core.antrea.tanzu.vmware.com/v1alpha2",
   "kind":"ClusterGroup",
   "metadata":{
      "annotations":{
         "kubectl.kubernetes.io/last-applied-configuration":"{\"apiVersion\":\"core.antrea.tanzu.vmware.com/v1alpha2\",\"kind\":\"ClusterGroup\",\"metadata\":{\"annotations\":{},\"name\":\"mycg\"},\"spec\":{\"podSelector\":{\"matchLabels\":{\"k8s-app\":\"kube-dns\"}}}}\n"
      },
      "creationTimestamp":"2021-01-29T19:59:42Z",
      "generation":1,
      "managedFields":[
         {
            "apiVersion":"core.antrea.tanzu.vmware.com/v1alpha2",
            "fieldsType":"FieldsV1",
            "fieldsV1":{
               "f:status":{
                  ".":{
                     
                  },
                  "f:conditions":{
                     
                  }
               }
            },
            "manager":"antrea-controller",
            "operation":"Update",
            "time":"2021-01-29T19:59:42Z"
         },
         {
            "apiVersion":"core.antrea.tanzu.vmware.com/v1alpha2",
            "fieldsType":"FieldsV1",
            "fieldsV1":{
               "f:metadata":{
                  "f:annotations":{
                     ".":{
                        
                     },
                     "f:kubectl.kubernetes.io/last-applied-configuration":{
                        
                     }
                  }
               },
               "f:spec":{
                  ".":{
                     
                  },
                  "f:podSelector":{
                     
                  }
               }
            },
            "manager":"kubectl-client-side-apply",
            "operation":"Update",
            "time":"2021-01-29T19:59:42Z"
         }
      ],
      "name":"mycg",
      "resourceVersion":"611970",
      "uid":"7d2d3d15-bd7d-4f24-a07e-c756bab47579"
   },
   "spec":{
      "podSelector":{
         "matchLabels":{
            "k8s-app":"kube-dns"
         }
      }
   },
   "status":{
      "conditions":[
         {
            "lastTransitionTime":"2021-01-29T19:59:39Z",
            "status":"True",
            "type":"GroupMembersComputed"
         }
      ]
   }
}

codecov-io · 2021-01-23T02:39:01Z

Codecov Report

❗ No coverage uploaded for pull request base (main@b9bd4bb). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1778   +/-   ##
=======================================
  Coverage        ?   62.00%           
=======================================
  Files           ?      192           
  Lines           ?    16387           
  Branches        ?        0           
=======================================
  Hits            ?    10161           
  Misses          ?     5174           
  Partials        ?     1052

Flag	Coverage Δ
e2e-tests	`26.56% <0.00%> (?)`
kind-e2e-tests	`49.64% <0.00%> (?)`
unit-tests	`42.83% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

pkg/controller/networkpolicy/clustergroup.go

pkg/controller/networkpolicy/networkpolicy_controller.go

pkg/apis/core/v1alpha2/types.go

jianjuns · 2021-01-26T01:57:35Z

pkg/apis/core/v1alpha2/types.go

+	// GroupPending means the Group has been accepted by the system, but it has not been processed by Antrea.
+	GroupPending GroupPhase = "Pending"
+	// GroupRealized means the Group has realized all of its GroupMembers.
+	GroupRealized GroupPhase = "Realized"


I feel "Realized" does not best fit Groups, as it just means the Group members are computed? But we want to use the same terms for all CRDs?

i considered "Processed" and "Computed". Was not sure about either so stick with what is already used in NetworkPolicyStatus

Or do you think this (computed) is like a condition? @tnqn

I just took a look at K8s API convention to understand the difference between phase and condition, looks like they are for same purpose and phase is a deprecated usage:

Some resources in the v1 API contain fields called phase, and associated message, reason, and other status fields. The pattern of using phase is deprecated. Newer API types should use conditions instead. Phase was essentially a state-machine enumeration field, that contradicted system-design principles and hampered evolution, since adding new enum values breaks backward compatibility.

https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#typical-status-properties

thanks for the link.. makes sense to transition to Conditions.. here i propose the following:

status: conditions: - type: GroupMembersReady status: "True"/"False" lastTransitionTime: "2021-1-25T16:29:31Z"

How about GroupMembersComputed?

How about GroupMembersComputed?

i don't have a strong opinion. I was now thinking of using the plain commonly used term Ready or Succeeded for type value, will post patch with GroupMembersComputed

Ok. No strong preference either. Would let you decide.

jianjuns

LGTM.

tnqn · 2021-01-29T02:55:01Z

pkg/controller/networkpolicy/clustergroup.go

+
+// groupMembersComputedConditionEqual checks whether the condition status for GroupMembersComputed condition
+// is same. Returns true if equal, otherwise returns false. It disregards the lastTransitionTime field.
+func groupMembersComputedConditionEqual(conds []corev1a2.GroupCondition, condition corev1a2.GroupCondition) bool {


Maybe later this function can be more generic to compare all desired conditions and actual condition, assuming the actual conditions should be in the same order of the desired conditions as they are updated to API in this order.

thanks.. will do in a follow up

abhiraut · 2021-01-29T03:30:46Z

/test-all

abhiraut · 2021-01-29T19:27:59Z

simple rebase to get some jobs passing

/test-all

abhiraut · 2021-01-29T20:43:41Z

@jianjuns @Dyanngg can i get another review, I had to rebase after Quan's review to get some jobs running successfully. Thanks.

abhiraut requested a review from tnqn January 23, 2021 02:34

vmwclabot added the cla-not-required label Jan 23, 2021

abhiraut force-pushed the cg-status branch from 86c8106 to 62a5493 Compare January 23, 2021 02:44

tnqn reviewed Jan 25, 2021

View reviewed changes

abhiraut force-pushed the cg-status branch 2 times, most recently from 73dc816 to 81fe802 Compare January 25, 2021 21:34

abhiraut commented Jan 25, 2021

View reviewed changes

pkg/apis/core/v1alpha2/types.go Outdated Show resolved Hide resolved

abhiraut requested review from tnqn, jianjuns and Dyanngg January 25, 2021 21:35

abhiraut mentioned this pull request Jan 25, 2021

Add ClusterGroup CRD to group Pods based on labels or share IPBlock #1782

Closed

12 tasks

Base automatically changed from master to main January 26, 2021 00:00

jianjuns reviewed Jan 26, 2021

View reviewed changes

abhiraut force-pushed the cg-status branch 5 times, most recently from 4cd3da1 to c29adef Compare January 27, 2021 23:07

abhiraut requested a review from jianjuns January 28, 2021 20:52

abhiraut force-pushed the cg-status branch from c29adef to 2cc544f Compare January 28, 2021 22:26

jianjuns previously approved these changes Jan 28, 2021

View reviewed changes

tnqn reviewed Jan 29, 2021

View reviewed changes

tnqn previously approved these changes Jan 29, 2021

View reviewed changes

abhiraut added 3 commits January 29, 2021 11:27

Add Status subresource to ClusterGroup CRD

f3ce4f4

Move all processing to sync handlers and set default status

0c4cc9b

Update Status to include Conditions

ce2ebb2

abhiraut dismissed stale reviews from tnqn and jianjuns via ce2ebb2 January 29, 2021 19:27

abhiraut force-pushed the cg-status branch from 2cc544f to ce2ebb2 Compare January 29, 2021 19:27

abhiraut requested a review from jianjuns January 29, 2021 20:42

abhiraut added this to the Antrea v0.13.0 release milestone Jan 29, 2021

jianjuns approved these changes Jan 29, 2021

View reviewed changes

abhiraut merged commit 606b161 into antrea-io:main Jan 29, 2021

abhiraut deleted the cg-status branch January 29, 2021 22:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Status subresource to ClusterGroup CRD #1778

Add Status subresource to ClusterGroup CRD #1778

abhiraut commented Jan 23, 2021 •

edited

codecov-io commented Jan 23, 2021 •

edited

jianjuns Jan 26, 2021

abhiraut Jan 26, 2021

jianjuns Jan 26, 2021

tnqn Jan 26, 2021

abhiraut Jan 26, 2021

jianjuns Jan 26, 2021

abhiraut Jan 26, 2021

jianjuns Jan 26, 2021

abhiraut Jan 26, 2021

jianjuns left a comment

tnqn Jan 29, 2021

abhiraut Jan 29, 2021 •

edited

abhiraut commented Jan 29, 2021

abhiraut commented Jan 29, 2021

abhiraut commented Jan 29, 2021

Add Status subresource to ClusterGroup CRD #1778

Add Status subresource to ClusterGroup CRD #1778

Conversation

abhiraut commented Jan 23, 2021 • edited

codecov-io commented Jan 23, 2021 • edited

Codecov Report

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

jianjuns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhiraut Jan 29, 2021 • edited

Choose a reason for hiding this comment

abhiraut commented Jan 29, 2021

abhiraut commented Jan 29, 2021

abhiraut commented Jan 29, 2021

abhiraut commented Jan 23, 2021 •

edited

codecov-io commented Jan 23, 2021 •

edited

abhiraut Jan 29, 2021 •

edited