You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "martinzink (via GitHub)" <gi...@apache.org> on 2023/05/16 12:50:19 UTC
[GitHub] [nifi-minifi-cpp] martinzink opened a new pull request, #1574: MINIFICPP-1641 Parallelization of docker tests
martinzink opened a new pull request, #1574:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1574
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically main)?
- [ ] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the LICENSE file?
- [ ] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1574: MINIFICPP-1641 Parallelization of docker tests
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1574:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1574#discussion_r1204305885
##########
docker/test/integration/cluster/containers/TcpClientContainer.py:
##########
@@ -29,9 +29,11 @@ def deploy(self):
logging.info('Creating and running a tcp client docker container...')
self.client.containers.run(
- self.image_store.get_image(self.get_engine()),
+ "alpine:3.17.3",
detach=True,
name=self.name,
network=self.network.name,
- entrypoint=self.command)
+ entrypoint='/bin/sh',
+ command="-c 'apk add netcat-openbsd && echo TCP client container started; while true; do echo "
+ f"test_tcp_message | nc minifi-cpp-flow-{self.context.feature_id} 10254; sleep 1; done'")
Review Comment:
It's in an endless `while` loop, sending a message to `minifi-cpp-flow-{feature_id}` port `10254`. The sleep is to avoid a busy loop, sending as many messages as the CPU can churn out.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1574: MINIFICPP-1641 Parallelization of docker tests
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1574:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1574#discussion_r1196441139
##########
docker/DockerVerify.sh:
##########
@@ -26,18 +26,19 @@ die()
}
_positionals=()
-_arg_feature_path=('' )
_arg_image_tag_prefix=
-_enable_test_processors=OFF
-
+_arg_tags_to_exclude=
+_arg_parallel_processes=3
Review Comment:
Further increasing the parallelism caused instability on the ci machines without providing any real reduction in runtime. (locally with more ram and threads worked great)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1574: MINIFICPP-1641 Parallelization of docker tests
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1574:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1574#discussion_r1223093553
##########
docker/test/integration/resources/minifi/minifi-log.properties:
##########
@@ -2,3 +2,4 @@ spdlog.pattern=[%Y-%m-%d %H:%M:%S.%e] [%n] [%l] %v
appender.stderr=stderr
logger.root=TRACE,stderr
logger.org::apache::nifi::minifi=TRACE,stderr
+logger.org::apache::nifi::minifi::SchedulingAgent=INFO,stderr
Review Comment:
good catch, I've reverted the changes to this file though https://github.com/apache/nifi-minifi-cpp/pull/1574/commits/b1355bf745e30cdec6973541b6249e55b368da08, since it will be fixed in MINIFICPP-2125
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1574: MINIFICPP-1641 Parallelization of docker tests
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1574:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1574#discussion_r1210440434
##########
docker/test/integration/ssl_utils/SSL_cert_utils.py:
##########
@@ -110,7 +110,7 @@ def make_ca(common_name):
return ca_cert, ca_key
-def make_cert(common_name, ca_cert, ca_key):
+def _make_cert(common_name, ca_cert, ca_key, is_server_auth=True, is_client_auth=True):
Review Comment:
why? (just asking, not arguing)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a diff in pull request #1574: MINIFICPP-1641 Parallelization of docker tests
Posted by "adamdebreceni (via GitHub)" <gi...@apache.org>.
adamdebreceni commented on code in PR #1574:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1574#discussion_r1204290125
##########
docker/test/integration/cluster/containers/TcpClientContainer.py:
##########
@@ -29,9 +29,11 @@ def deploy(self):
logging.info('Creating and running a tcp client docker container...')
self.client.containers.run(
- self.image_store.get_image(self.get_engine()),
+ "alpine:3.17.3",
detach=True,
name=self.name,
network=self.network.name,
- entrypoint=self.command)
+ entrypoint='/bin/sh',
+ command="-c 'apk add netcat-openbsd && echo TCP client container started; while true; do echo "
+ f"test_tcp_message | nc minifi-cpp-flow-{self.context.feature_id} 10254; sleep 1; done'")
Review Comment:
what is the `sleep 1` for?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1574: MINIFICPP-1641 Parallelization of docker tests
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1574:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1574#discussion_r1210416355
##########
docker/test/integration/features/MiNiFi_integration_test_driver.py:
##########
@@ -37,48 +40,68 @@
class MiNiFi_integration_test:
- def __init__(self, context):
- self.test_id = context.test_id
- self.cluster = DockerTestCluster(context)
+ def __init__(self, context, feature_id: str):
+ self.feature_id = feature_id
+ self.cluster = DockerTestCluster(context, feature_id=feature_id)
self.connectable_nodes = []
# Remote process groups are not connectables
self.remote_process_groups = []
self.file_system_observer = None
self.docker_directory_bindings = context.directory_bindings
- self.cluster.set_directory_bindings(self.docker_directory_bindings.get_directory_bindings(self.test_id), self.docker_directory_bindings.get_data_directories(self.test_id))
+ self.cluster.set_directory_bindings(self.docker_directory_bindings.get_directory_bindings(self.feature_id), self.docker_directory_bindings.get_data_directories(self.feature_id))
+ self.root_ca_cert, self.root_ca_key = make_ca("root CA")
+
+ minifi_client_cert, minifi_client_key = make_client_cert(common_name=f"minifi-cpp-flow-{self.feature_id}",
+ ca_cert=self.root_ca_cert,
+ ca_key=self.root_ca_key)
+ self.put_test_resource('root_ca.crt',
+ OpenSSL.crypto.dump_certificate(type=OpenSSL.crypto.FILETYPE_PEM,
+ cert=self.root_ca_cert))
+
+ self.put_test_resource('minifi_client.crt',
+ OpenSSL.crypto.dump_certificate(type=OpenSSL.crypto.FILETYPE_PEM,
+ cert=minifi_client_cert))
+ self.put_test_resource('minifi_client.key',
+ OpenSSL.crypto.dump_privatekey(type=OpenSSL.crypto.FILETYPE_PEM,
+ pkey=minifi_client_key))
+
+ def get_container_name_with_postfix(self, container_name: str):
+ return self.cluster.container_store.get_container_name_with_postfix(container_name)
def cleanup(self):
self.cluster.cleanup()
+ if self.file_system_observer:
+ self.file_system_observer.observer.unschedule_all()
- def acquire_container(self, name, engine='minifi-cpp', command=None):
- return self.cluster.acquire_container(name, engine, command)
+ def acquire_container(self, context, name, engine='minifi-cpp', command=None):
Review Comment:
Is it necessary to pass the whole behave context in all of these functions? It seems that only the `feature_id` and ca cert paths are used in the containers. We could have a smaller feature specific context that could be passed around containing only the required test data.
##########
docker/test/integration/ssl_utils/SSL_cert_utils.py:
##########
@@ -110,7 +110,7 @@ def make_ca(common_name):
return ca_cert, ca_key
-def make_cert(common_name, ca_cert, ca_key):
+def _make_cert(common_name, ca_cert, ca_key, is_server_auth=True, is_client_auth=True):
Review Comment:
This should be changed to only allow either server or client authentication but not both.
##########
docker/DockerVerify.sh:
##########
@@ -26,18 +26,19 @@ die()
}
_positionals=()
-_arg_feature_path=('' )
_arg_image_tag_prefix=
-_enable_test_processors=OFF
-
+_arg_tags_to_exclude=
+_arg_parallel_processes=3
Review Comment:
Can we add a cmake option for the --parallel-processes argument to change the default locally when calling the `make docker-verify` command?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1574: MINIFICPP-1641 Parallelization of docker tests
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1574:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1574#discussion_r1223095940
##########
docker/test/integration/ssl_utils/SSL_cert_utils.py:
##########
@@ -110,7 +110,7 @@ def make_ca(common_name):
return ca_cert, ca_key
-def make_cert(common_name, ca_cert, ca_key):
+def _make_cert(common_name, ca_cert, ca_key, is_server_auth=True, is_client_auth=True):
Review Comment:
refactored this in https://github.com/apache/nifi-minifi-cpp/pull/1574/commits/97ca6bab7beef928c28643f5d7a14e6022605016
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on pull request #1574: MINIFICPP-1641 Parallelization of docker tests
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on PR #1574:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1574#issuecomment-1552017146
force pushed a different commit with identical contents to trigger the CI workflow
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1574: MINIFICPP-1641 Parallelization of docker tests
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1574:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1574#discussion_r1211586133
##########
docker/test/integration/ssl_utils/SSL_cert_utils.py:
##########
@@ -110,7 +110,7 @@ def make_ca(common_name):
return ca_cert, ca_key
-def make_cert(common_name, ca_cert, ca_key):
+def _make_cert(common_name, ca_cert, ca_key, is_server_auth=True, is_client_auth=True):
Review Comment:
Even if it is only used internally the signature allows us to have both server and client auth that should not be possible, which is also the default.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1574: MINIFICPP-1641 Parallelization of docker tests
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #1574:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1574#discussion_r1196033405
##########
docker/DockerVerify.sh:
##########
@@ -26,18 +26,19 @@ die()
}
_positionals=()
-_arg_feature_path=('' )
_arg_image_tag_prefix=
-_enable_test_processors=OFF
-
+_arg_tags_to_exclude=
+_arg_parallel_processes=3
Review Comment:
Can we increase the parallelism to further reduce runtimes, or are we running into some other bottleneck?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] lordgamez commented on a diff in pull request #1574: MINIFICPP-1641 Parallelization of docker tests
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #1574:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1574#discussion_r1210454963
##########
docker/test/integration/ssl_utils/SSL_cert_utils.py:
##########
@@ -110,7 +110,7 @@ def make_ca(common_name):
return ca_cert, ca_key
-def make_cert(common_name, ca_cert, ca_key):
+def _make_cert(common_name, ca_cert, ca_key, is_server_auth=True, is_client_auth=True):
Review Comment:
This was an issue when upgrading to OpenSSL 3, when both were used the SSL handshake failed, only the client or server specific auth should be added. (Sorry I didn't elaborate before, we had this discussion previously with @martinzink in person :))
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1574: MINIFICPP-1641 Parallelization of docker tests
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1574:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1574#discussion_r1211397616
##########
docker/test/integration/ssl_utils/SSL_cert_utils.py:
##########
@@ -110,7 +110,7 @@ def make_ca(common_name):
return ca_cert, ca_key
-def make_cert(common_name, ca_cert, ca_key):
+def _make_cert(common_name, ca_cert, ca_key, is_server_auth=True, is_client_auth=True):
Review Comment:
I thought the _ prefix (aka weak “internal use” indicator) should discourage anyone to use this function externally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1574: MINIFICPP-1641 Parallelization of docker tests
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1574:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1574#discussion_r1223095139
##########
docker/DockerVerify.sh:
##########
@@ -26,18 +26,19 @@ die()
}
_positionals=()
-_arg_feature_path=('' )
_arg_image_tag_prefix=
-_enable_test_processors=OFF
-
+_arg_tags_to_exclude=
+_arg_parallel_processes=3
Review Comment:
good idea, I've added a new option and set it to default to the number of threads on the host
https://github.com/apache/nifi-minifi-cpp/pull/1574/commits/47c55d663e42c90be2debf171314d888a4639bc4
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] martinzink commented on a diff in pull request #1574: MINIFICPP-1641 Parallelization of docker tests
Posted by "martinzink (via GitHub)" <gi...@apache.org>.
martinzink commented on code in PR #1574:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1574#discussion_r1223096958
##########
docker/test/integration/features/MiNiFi_integration_test_driver.py:
##########
@@ -37,48 +40,68 @@
class MiNiFi_integration_test:
- def __init__(self, context):
- self.test_id = context.test_id
- self.cluster = DockerTestCluster(context)
+ def __init__(self, context, feature_id: str):
+ self.feature_id = feature_id
+ self.cluster = DockerTestCluster(context, feature_id=feature_id)
self.connectable_nodes = []
# Remote process groups are not connectables
self.remote_process_groups = []
self.file_system_observer = None
self.docker_directory_bindings = context.directory_bindings
- self.cluster.set_directory_bindings(self.docker_directory_bindings.get_directory_bindings(self.test_id), self.docker_directory_bindings.get_data_directories(self.test_id))
+ self.cluster.set_directory_bindings(self.docker_directory_bindings.get_directory_bindings(self.feature_id), self.docker_directory_bindings.get_data_directories(self.feature_id))
+ self.root_ca_cert, self.root_ca_key = make_ca("root CA")
+
+ minifi_client_cert, minifi_client_key = make_client_cert(common_name=f"minifi-cpp-flow-{self.feature_id}",
+ ca_cert=self.root_ca_cert,
+ ca_key=self.root_ca_key)
+ self.put_test_resource('root_ca.crt',
+ OpenSSL.crypto.dump_certificate(type=OpenSSL.crypto.FILETYPE_PEM,
+ cert=self.root_ca_cert))
+
+ self.put_test_resource('minifi_client.crt',
+ OpenSSL.crypto.dump_certificate(type=OpenSSL.crypto.FILETYPE_PEM,
+ cert=minifi_client_cert))
+ self.put_test_resource('minifi_client.key',
+ OpenSSL.crypto.dump_privatekey(type=OpenSSL.crypto.FILETYPE_PEM,
+ pkey=minifi_client_key))
+
+ def get_container_name_with_postfix(self, container_name: str):
+ return self.cluster.container_store.get_container_name_with_postfix(container_name)
def cleanup(self):
self.cluster.cleanup()
+ if self.file_system_observer:
+ self.file_system_observer.observer.unschedule_all()
- def acquire_container(self, name, engine='minifi-cpp', command=None):
- return self.cluster.acquire_container(name, engine, command)
+ def acquire_container(self, context, name, engine='minifi-cpp', command=None):
Review Comment:
good idea, I've introduced FeatureContext in https://github.com/apache/nifi-minifi-cpp/pull/1574/commits/94d5a6fc0a0263283376dcfa7d46d655b22e1828 and https://github.com/apache/nifi-minifi-cpp/pull/1574/commits/69e035b345f3298650fa482fc74227d4fb080271
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] szaszm commented on pull request #1574: MINIFICPP-1641 Parallelization of docker tests
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on PR #1574:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1574#issuecomment-1550867564
Interestingly, the Github Actions CI checks didn't run on this PR. Is it turned off somewhere, or was it just the Github Infra having trigger issues again?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] fgerlits closed pull request #1574: MINIFICPP-1641 Parallelization of docker tests
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits closed pull request #1574: MINIFICPP-1641 Parallelization of docker tests
URL: https://github.com/apache/nifi-minifi-cpp/pull/1574
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a diff in pull request #1574: MINIFICPP-1641 Parallelization of docker tests
Posted by "adamdebreceni (via GitHub)" <gi...@apache.org>.
adamdebreceni commented on code in PR #1574:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1574#discussion_r1204366908
##########
docker/test/integration/resources/minifi/minifi-log.properties:
##########
@@ -2,3 +2,4 @@ spdlog.pattern=[%Y-%m-%d %H:%M:%S.%e] [%n] [%l] %v
appender.stderr=stderr
logger.root=TRACE,stderr
logger.org::apache::nifi::minifi=TRACE,stderr
+logger.org::apache::nifi::minifi::SchedulingAgent=INFO,stderr
Review Comment:
missing newline at the end of file?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org