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

[VCDA-2578, VCDA-2108, VCDA-2262] Cluster share fix and system user cluster sharing #1083

Merged
merged 14 commits into from
Jun 28, 2021

Conversation

ltimothy7
Copy link
Contributor

@ltimothy7 ltimothy7 commented Jun 22, 2021

Signed-off-by: ltimothy ltimothy@vmware.com

To help us process your pull request efficiently, please include:

  • (Required) Detailed description of changes include tests and
    documentation. If the pull request contains multiple commits with
    detailed messages, refer to those instead
    Tested sharing, unsharing, and sharing list for native and TKG system and non-system users

  • (Optional) Names of reviewers using @ sign + name


This change is Reviewable

Signed-off-by: ltimothy <ltimothy@vmware.com>
Signed-off-by: ltimothy <ltimothy@vmware.com>
…ce-extension into cluster_share_fix

Signed-off-by: ltimothy <ltimothy@vmware.com>
…ce-extension into cluster_share_fix

Signed-off-by: ltimothy <ltimothy@vmware.com>
Signed-off-by: ltimothy <ltimothy@vmware.com>
Signed-off-by: ltimothy <ltimothy@vmware.com>
…ce-extension into cluster_share_fix

Signed-off-by: ltimothy <ltimothy@vmware.com>
…ce-extension into cluster_share_fix

Signed-off-by: ltimothy ltimothy@vmware.com
Copy link
Contributor

@Anirudh9794 Anirudh9794 left a comment

Choose a reason for hiding this comment

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

:lgtm: minor question.

Reviewed 19 of 19 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ltimothy7)


container_service_extension/rde/models/common_models.py, line 144 at r1 (raw file):

    owner: Optional[Owner] = None
    org: Optional[Org] = None
    metadataSummary: Optional[list] = None

Are we not ignoring metadataSummary ?
Is DefEntity(**entity_dict) raising an exception?

…ce-extension into cluster_share_fix

Signed-off-by: ltimothy <ltimothy@vmware.com>
Signed-off-by: ltimothy <ltimothy@vmware.com>
Signed-off-by: ltimothy <ltimothy@vmware.com>
@vmwclabot
Copy link
Member

@ltimothy7, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <john.doe@email.org> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

@ltimothy7
Copy link
Contributor Author


container_service_extension/rde/models/common_models.py, line 144 at r1 (raw file):

Previously, Anirudh9794 wrote…

Are we not ignoring metadataSummary ?
Is DefEntity(**entity_dict) raising an exception?

Thanks Aniruddha, This is no longer needed, it was raising an exception

@Anirudh9794
Copy link
Contributor


container_service_extension/common/utils/pyvcloud_utils.py, line 663 at r2 (raw file):

            client=client,
            resource=org_obj)
        if org.get_vdc(vdc_name) is not None:

This function can possibly become really costly.There were customers who had thousands of orgs and thousands of vdcs in each org. I think it is better to use typed query like
https://github.com/vmware/pyvcloud/blob/master/pyvcloud/vcd/client.py#L1599
instead of iterating over orgs

Signed-off-by: ltimothy <ltimothy@vmware.com>
@vmwclabot
Copy link
Member

@ltimothy7, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <john.doe@email.org> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

Signed-off-by: ltimothy <ltimothy@vmware.com>
@vmwclabot
Copy link
Member

@ltimothy7, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <john.doe@email.org> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

Copy link
Collaborator

@sahithi sahithi left a comment

Choose a reason for hiding this comment

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

:lgtm: (few comments)
Tim, I could not review the correctness of the logical workflow as I don't remember the full context. I suggest demonstrating this piece in our next demo as a refresher to all of us.

Reviewable status: 10 of 19 files reviewed, 4 unresolved discussions (waiting on @Anirudh9794 and @ltimothy7)


container_service_extension/common/constants/shared_constants.py, line 84 at r4 (raw file):

# System org constants
SYSTEM_ORG_NAME = 'system'

Don't we already have System constant declared somewhere?


container_service_extension/rde/models/common_models.py, line 190 at r4 (raw file):

@dataclass()
class ClusterAclEntry:

I suggest using dataclass_json annotation to enable snake case fields in the python code.
This will enable easy conversions between JSON payloads.

Please add a TODO if you want to take this task up later.

Signed-off-by: ltimothy <ltimothy@vmware.com>
@vmwclabot
Copy link
Member

@ltimothy7, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <john.doe@email.org> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

Copy link
Contributor Author

@ltimothy7 ltimothy7 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 10 of 19 files reviewed, 4 unresolved discussions (waiting on @Anirudh9794 and @sahithi)


container_service_extension/common/constants/shared_constants.py, line 84 at r4 (raw file):

Previously, sahithi (Sahithi Ayloo) wrote…

Don't we already have System constant declared somewhere?

Yes, I needed to move it to shared_constants so that de_cluster_tkg could use the constant


container_service_extension/common/utils/pyvcloud_utils.py, line 663 at r2 (raw file):

Previously, Anirudh9794 wrote…

This function can possibly become really costly.There were customers who had thousands of orgs and thousands of vdcs in each org. I think it is better to use typed query like
https://github.com/vmware/pyvcloud/blob/master/pyvcloud/vcd/client.py#L1599
instead of iterating over orgs

Done. Thanks Aniruddha


container_service_extension/rde/models/common_models.py, line 190 at r4 (raw file):

Previously, sahithi (Sahithi Ayloo) wrote…

I suggest using dataclass_json annotation to enable snake case fields in the python code.
This will enable easy conversions between JSON payloads.

Please add a TODO if you want to take this task up later.

Done. Thank you, this is now a dataclass_json

@ltimothy7 ltimothy7 merged commit a41ecd9 into vmware:master Jun 28, 2021
@ltimothy7 ltimothy7 deleted the cluster_share_fix branch June 30, 2022 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants