Added dashes to pod naming

[SimonOsipov]

Closes: #16600

In order to have a more readable naming of KubernetesExecutor Pod, decided to remove deleting "dashes" as per the documentation it is allowed.

[boring-cyborg]

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

• Pay attention to the quality of your code (flake8, pylint and type annotations). Our pre-commits will help you with that.
• In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
• Consider using Breeze environment for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
• Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
• Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
• Be sure to read the Airflow Coding style.
  Apache Airflow is a community-driven project and together we are making it better 🚀.
  In case of doubts contact the developers at:
  Mailing List: dev@airflow.apache.org
  Slack: https://s.apache.org/airflow-slack

[uranusjr]

Some test cases will also be needed.

[uranusjr]

It’s probably easier to do a re.sub instead.

[SimonOsipov]

Can you elaborate more on why using an additional module would be more effective? I am fairly new to Python, so I am eager for knowledge.

[uranusjr]

Not many reasons TBH, just it’s probably easier to understand IMO.

return re.sub(r"[^-a-z0-9]+", string.lower())

It may also be easier to implement additional logic, such as avoiding - in the beginning of the string (which the logic you implement right now does not do).

[houqp]

if we are to use regex, we should use a precompiled regex object. If we are not going to use regex here, then i think we should just do the comparison with ch == '-' without creating a new list in memory.

[SimonOsipov]

Some test cases will also be needed.

airflow/tests/kubernetes/test_pod_generator.py

Lines 650 to 679 in 86d0a96

	def test_pod_name_confirm_to_max_length(self, _, pod_id):
	name = PodGenerator.make_unique_pod_id(pod_id)
	assert len(name) <= 253
	parts = name.split(".")
	if len(pod_id) <= 63:
	assert len(parts[0]) == len(pod_id)
	else:
	assert len(parts[0]) <= 63
	assert len(parts[1]) <= 63
	@parameterized.expand(
	(
	("pod-name-with-hyphen-", "pod-name-with-hyphen"),
	("pod-name-with-double-hyphen--", "pod-name-with-double-hyphen"),
	("pod0-name", "pod0-name"),
	("simple", "simple"),
	("pod-name-with-dot.", "pod-name-with-dot"),
	("pod-name-with-double-dot..", "pod-name-with-double-dot"),
	("pod-name-with-hyphen-dot-.", "pod-name-with-hyphen-dot"),
	)
	)
	def test_pod_name_is_valid(self, pod_id, expected_starts_with):
	name = PodGenerator.make_unique_pod_id(pod_id)
	regex = r"^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$"
	assert (
	len(name) <= 253 and all(ch.lower() == ch for ch in name) and re.match(regex, name)
	), "pod_id is invalid - fails allowed regex check"
	assert name.rsplit(".")[0] == expected_starts_with

It seems like already provided test cases are enough (they are with dashes). Also, regex is valid for using dashes.

[uranusjr]

It seems like already provided test cases are enough

Some more should be added since the logic now has more allowed characters. Name with leading dash/dot/underscore, for example.

[kaxil]

There were also #15443 and #15445

'-' followed by '.' isn't allowed which caused issues when pod_id was trimmed

cc @houqp

[houqp]

+1 on what @kaxil said, i don't see specific code to guard against that regression?

[houqp]

if we are to use regex, we should use a precompiled regex object. If we are not going to use regex here, then i think we should just do the comparison with ch == '-' without creating a new list in memory.

[SimonOsipov]

I am sorry, have been sick lately, can we return to this MR?
What was the decision and what additional things needs to be done to implement this (I would like to take and complete them) =)

@uranusjr @kaxil @houqp

[houqp]

@SimonOsipov i think you need to write code to check and remove trailing - after the truncation so when we concat them with ., it won't result in invalid pod name in k8s. And of course add some unit tests for it :)

[uranusjr]

I think #17057 covers this.

[uranusjr]

#17057 only covers the kubernetes_helper_functions.py part of this PR; we still need other parts of this.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

[SimonOsipov]

I terribly apologize, I a bit felt out of the issue I created. Is this issue still valid?

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.