Add ovdc, system, template request handlers

[andrew-ni]

• Created request handler files for ovdc, system, and template requests (under /request_handlers/)
• Removed unnecessary classes and turned them into functions (ServiceProcessor, OvdcManager)
• OvdcManager functionality has been moved to ovdc_utils.py, and some functions have been reworked/streamlined. Some of these functions still use the request data and pass around dictionaries. This will need to be further refactored.
• Processor maintains an operation to handler mapping to clearly decouple processor from doing any operational functionality. The only thing processor should be concerned with is parsing data into a single dictionary, and passing it to the relevant handler.
• request_handler/x_handler.py files are CSE's request data validators and functional interfaces, where specific request data is used as parameters in generic functions. These handlers can also manipulate return data received by the generic functions depending on the operation's needs
• Fixed the bug where using vcd cse ovdc list -p outputs pks info for non-pks ovdcs

Testing passes (169)

---

This change is 

[rocknes]

Reviewed 16 of 18 files at r1, 1 of 2 files at r2.
Reviewable status: 17 of 18 files reviewed, 12 unresolved discussions (waiting on @andrew-ni, @harshneelmore, @rocknes, and @sakthisunda)

---

container_service_extension/broker_manager.py, line 156 at r2 (raw file):

                                            f"already exists.")

        ctr_prov_ctx = ovdc_utils.get_ovdc_k8s_provider_metadata(org_name=org_name, ovdc_name=vdc_name, get_credentials=True, get_nsxt_info=True) # noqa: E501

I don't think the credentials or nsxt details are needed here.
The context is not sent to the actual method creating the cluster.
Furthermore the logic to find the broker from cnontext (vdc metadata) should based on finding the vc backing the vdc and then querrying pks cache to get the relevant info. If it's being done in a different way, we shoul re-evaluate.

---

container_service_extension/broker_manager.py, line 173 at r2 (raw file):

        If 'ovdc' is present in the cluster spec,
            choose the right broker (by identifying the k8s provider

I think PM wanted k8s to be replaced by container everywhere. Maybe ask Aashima for the guideline.

---

container_service_extension/consumer.py, line 15 at r2 (raw file):

from container_service_extension.logger import SERVER_LOGGER as LOGGER
import container_service_extension.processor as processor

nit: request_processor?

---

container_service_extension/cse.py, line 109 at r2 (raw file):

def version(ctx):
    """Display CSE version."""
    from container_service_extension.service import Service

Why the localalized import?

Localized imports are tell tale sign of bad code partitioning.

---

container_service_extension/ovdc_manager.py, line 1 at r2 (raw file):

# container-service-extension

Delete the file instead of blanking it out.

---

container_service_extension/ovdc_request_handler.py, line 1 at r2 (raw file):

# container-service-extension

delete the file instead of blanking it out.

---

container_service_extension/pksbroker.py, line 98 at r2 (raw file):

                "PKS context is required to establish connection to PKS")
        from pprint import pprint

stray print statement.

---

container_service_extension/pksbroker_manager.py, line 127 at r2 (raw file):

                ctr_prov_ctx = ovdc_utils.get_ovdc_k8s_provider_metadata(
                    ovdc_name=vdc_name, org_name=org_name,
                    get_credentials=True)

I will propose, we name the variable(s) get_X as include_X, which conveys the intention better in my opinion.

---

container_service_extension/processor.py, line 243 at r2 (raw file):

    reply['status_code'] = operation.ideal_response_code
    try:
        reply['body'] = OPERATION_TO_HANDLER[operation](request_data,

neat :-)

---

container_service_extension/processor.py, line 250 at r2 (raw file):

        pass
    # TODO all cluster operations should route to cluster_handler.py
    from container_service_extension.broker_manager import BrokerManager

Localized imports generally mean there is an underlying problem with code organization, do you know the exact import loop that forced you to move the import inline?

---

container_service_extension/utils.py, line 296 at r2 (raw file):

def check_keys_in_dikt(required_keys, dikt, dict_name='dictionary'):

nit: keep this method next to check_keys_and_values

---

container_service_extension/request_handlers/ovdc_handler.py, line 25 at r2 (raw file):

        utils.check_keys_in_dikt(required, request_dict, dict_name='request')

    k8s_provider_info = ovdc_utils.construct_ctr_prov_ctx_from_pks_cache(

If the system is not enabled for pks why should we consult pks cache?

I think it's much cleaner to branch off based on container provider, native = just 1 key-value pair, none = strip all relevant metadata, pks = craft the full blown dict using pks cache.

---

container_service_extension/request_handlers/ovdc_handler.py, line 34 at r2 (raw file):

    if request_dict.get(RequestKey.K8S_PROVIDER) == K8sProvider.PKS:
        if not utils.is_pks_enabled():
            raise CseServerError('CSE is not configured to work with PKS.')

Fail as early as possible.

[andrew-ni]

Reviewable status: 17 of 18 files reviewed, 10 unresolved discussions (waiting on @harshneelmore, @rocknes, and @sakthisunda)

---

container_service_extension/broker_manager.py, line 156 at r2 (raw file):

Previously, rocknes wrote…

I don't think the credentials or nsxt details are needed here.
The context is not sent to the actual method creating the cluster.
Furthermore the logic to find the broker from cnontext (vdc metadata) should based on finding the vc backing the vdc and then querrying pks cache to get the relevant info. If it's being done in a different way, we shoul re-evaluate.

construct_ctr_prov_ctx_from_ovdc_metadata() was a function that just called the previous get_ovdc_k8s_provider_metadata with these two boolean flags = true. I've removed that function and replaced it with the appropriate get_ovdc_k8s_provider_metadata call and parameters. With that in mind though what do you think about this?

---

container_service_extension/broker_manager.py, line 173 at r2 (raw file):

Previously, rocknes wrote…

I think PM wanted k8s to be replaced by container everywhere. Maybe ask Aashima for the guideline.

hmm, i'll leave it for this review but i'll get that cleared up. Hopefully we can stay with k8s_provider because it's more accurate than container_provider (we're not providing containers, but rather entire kubernetes clusters). Also it's a lot shorter than container_provider everywhere

---

container_service_extension/cse.py, line 109 at r2 (raw file):

Previously, rocknes wrote…

Why the localalized import?

Localized imports are tell tale sign of bad code partitioning.

I remember there being a circular import issue, but that may have gotten fixed in the changes. doesn't seem to be failing anymore, moved it back up

---

container_service_extension/ovdc_manager.py, line 1 at r2 (raw file):

Previously, rocknes wrote…

Delete the file instead of blanking it out.

github shows its deleted, not sure why reviewable shows it like this

---

container_service_extension/ovdc_request_handler.py, line 1 at r2 (raw file):

Previously, rocknes wrote…

delete the file instead of blanking it out.

github shows its deleted, not sure why reviewable shows it like this

---

container_service_extension/pksbroker.py, line 98 at r2 (raw file):

Previously, rocknes wrote…

stray print statement.

Done.

---

container_service_extension/pksbroker_manager.py, line 127 at r2 (raw file):

Previously, rocknes wrote…

I will propose, we name the variable(s) get_X as include_X, which conveys the intention better in my opinion.

Done.

---

container_service_extension/processor.py, line 250 at r2 (raw file):

Previously, rocknes wrote…

Localized imports generally mean there is an underlying problem with code organization, do you know the exact import loop that forced you to move the import inline?

I put this here because cluster_handler.py is not included in this PR. Once it is, this import and chunk of code will be removed and the try/except loop contents will be moved out.

When cluster_handler.py is in, this processor code will look like:

reply['body'] = OPERATION_TO_HANDLER[operation](request_data, tenant_auth_token)
LOGGER.debug(f"reply: {str(reply)}")
return reply```

---

container_service_extension/request_handlers/ovdc_handler.py, line 25 at r2 (raw file):

Previously, rocknes wrote…

If the system is not enabled for pks why should we consult pks cache?

I think it's much cleaner to branch off based on container provider, native = just 1 key-value pair, none = strip all relevant metadata, pks = craft the full blown dict using pks cache.

fixed.

ovdc_utils.construct_ctr_prov_ctx_from_pks_cache() is weird because it internally has a PKS check, and if PKS is not active, it only returns

{'k8s_provider': k8s_provider}

---

container_service_extension/request_handlers/ovdc_handler.py, line 34 at r2 (raw file):

Previously, rocknes wrote…

Fail as early as possible.

Done.

[vmwclabot]

@andrew-ni, 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.

• Commit 00a630a must be signed by niandrew7@gmail.com

[vmwclabot]

@andrew-ni, 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.

• Commit 00a630a must be signed by niandrew7@gmail.com

[sakthisunda]

Reviewed 1 of 2 files at r2.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @harshneelmore and @rocknes)

[rocknes]

Reviewed 8 of 9 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @andrew-ni, @harshneelmore, and @rocknes)

---

container_service_extension/broker_manager.py, line 156 at r2 (raw file):

Previously, andrew-ni (Andrew Ni) wrote…

construct_ctr_prov_ctx_from_ovdc_metadata() was a function that just called the previous get_ovdc_k8s_provider_metadata with these two boolean flags = true. I've removed that function and replaced it with the appropriate get_ovdc_k8s_provider_metadata call and parameters. With that in mind though what do you think about this?

If they are working fine, then let's keep the flag values intact for now and revisit once we pick up the context refactor story.
+1 on the name change :)

---

container_service_extension/broker_manager.py, line 173 at r2 (raw file):

Previously, andrew-ni (Andrew Ni) wrote…

hmm, i'll leave it for this review but i'll get that cleared up. Hopefully we can stay with k8s_provider because it's more accurate than container_provider (we're not providing containers, but rather entire kubernetes clusters). Also it's a lot shorter than container_provider everywhere

Sure

---

container_service_extension/ovdc_utils.py, line 98 at r4 (raw file):

                                      'only for System Administrator.')
    if list_pks_plans and (request_dict is None or tenant_auth_token is None):
        raise ValueError("Request_dict and tenant_auth_token required "

This message will show up on user console, I don't think we should mention request_dict in such a message.

---

container_service_extension/ovdc_utils.py, line 334 at r4 (raw file):

class PksComputeProfileParams(namedtuple("PksComputeProfileParams",

PKS Compute profile logic should probably be moved to a different file, feel free to do so in a different PR.

---

container_service_extension/request_handlers/ovdc_handler.py, line 44 at r4 (raw file):

                                              tenant_auth_token, request_dict)

    task = ovdc_utils.set_ovdc_k8s_provider_metadata(

Please verify that we are stripping away the non required metadata inside this function if k8s_provider is not pks :)

[rocknes]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @andrew-ni and @harshneelmore)

---

container_service_extension/ovdc_request_handler.py, line 1 at r2 (raw file):

Previously, andrew-ni (Andrew Ni) wrote…

github shows its deleted, not sure why reviewable shows it like this

Did you git add after you actually deleted it?
I have a feeling that your IDE is doing things differently under the hood :(

[andrew-ni]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @harshneelmore and @rocknes)

---

container_service_extension/ovdc_request_handler.py, line 1 at r2 (raw file):

Previously, rocknes wrote…

Did you git add after you actually deleted it?
I have a feeling that your IDE is doing things differently under the hood :(

yeah I did, reviewable also shows that it's deleted. I'm seeing File not in pull request at selected revision. here.

---

container_service_extension/ovdc_utils.py, line 98 at r4 (raw file):

Previously, rocknes wrote…

This message will show up on user console, I don't think we should mention request_dict in such a message.

Done.

---

container_service_extension/ovdc_utils.py, line 334 at r4 (raw file):

Previously, rocknes wrote…

PKS Compute profile logic should probably be moved to a different file, feel free to do so in a different PR.

Done.

---

container_service_extension/request_handlers/ovdc_handler.py, line 44 at r4 (raw file):

Previously, rocknes wrote…

Please verify that we are stripping away the non required metadata inside this function if k8s_provider is not pks :)

if k8s_provider is not pks, this function only passes in the k8s_provider (native or none)

[vmwclabot]

@andrew-ni, 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.

• Commit 00a630a must be signed by niandrew7@gmail.com

[vmwclabot]

@andrew-ni, 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.

• Commit 0581bf7 must be signed by niandrew7@gmail.com

[vmwclabot]

@andrew-ni, 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.

• Commit 0581bf7 must be signed by niandrew7@gmail.com

[rocknes]

Reviewed 3 of 3 files at r6, 2 of 3 files at r7.
Reviewable status: 17 of 18 files reviewed, all discussions resolved (waiting on @harshneelmore)

[rocknes]

Reviewed 1 of 3 files at r7.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @andrew-ni and @harshneelmore)

---

system_tests/test_cse_server.py, line 361 at r7 (raw file):

Quoted 11 lines of code…

        try:
            ssh_client.connect(ip, username='root')
            max_tries = 4
            for i in range(max_tries):
                time.sleep(env.WAIT_INTERVAL)
                try:
                    ssh_client.connect(ip, username='root')
                    break
                except Exception:
                    if i == max_tries - 1:
                        raise

Can be refactored into

    try:
        max_tries = 5
        for i in range(max_tries):
            try:
                ssh_client.connect(ip, username='root')
                break
            except Exception:
                time.sleep(env.WAIT_INTERVAL)
                if i == max_tries - 1:
                    raise

thus avoid two separate instances of ssh_client.connect

[vmwclabot]

@andrew-ni, 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.

• Commit 0581bf7 must be signed by niandrew7@gmail.com

[andrew-ni]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @harshneelmore and @rocknes)

---

system_tests/test_cse_server.py, line 361 at r7 (raw file):

Previously, rocknes wrote…

        try:
            ssh_client.connect(ip, username='root')
            max_tries = 4
            for i in range(max_tries):
                time.sleep(env.WAIT_INTERVAL)
                try:
                    ssh_client.connect(ip, username='root')
                    break
                except Exception:
                    if i == max_tries - 1:
                        raise

Can be refactored into

    try:
        max_tries = 5
        for i in range(max_tries):
            try:
                ssh_client.connect(ip, username='root')
                break
            except Exception:
                time.sleep(env.WAIT_INTERVAL)
                if i == max_tries - 1:
                    raise

thus avoid two separate instances of ssh_client.connect

yeah mb, fixed. didn't notice that I didn't remove the original ssh_client.connect

[vmwclabot]

@andrew-ni, 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.

• Commit 0581bf7 must be signed by niandrew7@gmail.com

[rocknes]

Reviewed 1 of 1 files at r9.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @harshneelmore)

[vmwclabot]

@andrew-ni, 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.

• Commit 0581bf7 must be signed by niandrew7@gmail.com