[FLINK-16600][k8s] Fix not respecting the RestOptions.BIND_PORT for the Kubernetes setup

[zhengcanbin]

What is the purpose of the change

Our current logic only takes care of RestOptions.PORT but not RestOptions.BIND_PORT, which is a bug; for example, when one sets the RestOptions.BIND_PORT to a value different from RestOptions.PORT, jobs could not be submitted to the existing session cluster deployed via the kubernetes-session.sh.

This PR fixes the bug.

Brief change log

Set the RestOptions.BIND_PORT as the target port of the external Service.

Verifying this change

This change can be tested via the existing unit tests.

Does this pull request potentially affect one of the following parts:

• Dependencies (does it add or upgrade a dependency): (no)
• The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
• The serializers: (yes / no / don't know)
• The runtime per-record code paths (performance sensitive): (no)
• Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn/Mesos, ZooKeeper: (no)
• The S3 file system connector: (no)

Documentation

• Does this pull request introduce a new feature? (no)

[flinkbot]

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 005f751 (Sat Apr 11 02:56:04 UTC 2020)

Warnings:

• No documentation files were touched! Remember to keep the Flink docs up to date!

_{Mention the bot in a comment to re-run the automated checks.}

Review Progress

• ❓ 1. The [description] looks good.
• ❓ 2. There is [consensus] that the contribution should go into to Flink.
• ❓ 3. Needs [attention] from.
• ❓ 4. The change fits into the overall [architecture].
• ❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required

Bot commands

The @flinkbot bot supports the following commands:

• @flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
• @flinkbot approve all to approve all aspects
• @flinkbot approve-until architecture to approve everything until architecture
• @flinkbot attention @username1 [@username2 ..] to require somebody's attention
• @flinkbot disapprove architecture to remove an approval you gave earlier

[flinkbot]

CI report:

• 75e3de0 Travis: SUCCESS Azure: FAILURE

Bot commands

The @flinkbot bot supports the following commands:

• @flinkbot run travis re-run the last Travis build
• @flinkbot run azure re-run the last Azure build

[zhengcanbin]

@flinkbot run travis

[zhengcanbin]

@flinkbot run azure

[zhengcanbin]

@tisonkun @tillrohrmann @wangyang0918 Could you help take a look at this PR? Thanks a lot in advance！

[tisonkun]

I don't think such a change verified by "existing" tests. If the configuration isn't respect, you should point out/add a specific test failed without this change and pass with it.

[wangyang0918]

The changes looks good to me. I agree with @tisonkun that we need to add a new test, which will fails before the changes and pass after it.
For the test, i think we could have the following assumption.

• The port of rest service should be equal to rest.port.
• The targetPort should be equal to rest.bind-port.

[zhengcanbin]

Hi, @tisonkun @wangyang0918 Thanks for your review and comments.

The test case of ExternalServiceDecoratorTest#testBuildAccompanyingKubernetesResources already covers the test branch; before this PR, Line 72 has checked that the service port equals to the rest.port; after this PR, Line 73 checked that the service target port equals to the rest.bind-port, and if we don't add this line, ExternalServiceDecoratorTest#testBuildAccompanyingKubernetesResources will definitely fail after this change.

[tisonkun]

Could you add a new test and make sure we test with and without setting BIND_PORT.

As you change existing test, our test coverage has been downgraded.

[tisonkun]

With a quick offline discussion with @zhengcanbin I get the point that the existing test org.apache.flink.kubernetes.kubeclient.decorators.ExternalServiceDecoratorTest#testBuildAccompanyingKubernetesResources will fail with production code changes because we now always set targetPort which follows K8s best practice.

So the change of existing test is for respect this change and accidentally we configure BIND_PORT so that verify exactly the BIND_PORT is set. The fallback logic should be guarded by RestOption's tests themselves.

@wangyang0918 does this explanation make sense to you?

[wangyang0918]

@tisonkun @zhengcanbin I have checked the test and changes again. If we set rest.bind-port explicitly, the test testBuildAccompanyingKubernetesResources should fail. So updating the test seems reasonable.

Now i give the pass, +1 for merging.