✨🐛 Follow-up removing long running http requests from dy-sidecar (⚠️ devops) #3232

GitHK · 2022-08-03T07:26:37Z

⚠️ devops

Add graylog mail alert for Could not contact dynamic-sidecar to save service state or upload outputs when deploying this

What do these changes do?

dynamic-sidecar

adds cli option to the dynamic-sidecar to save the state directories (usually workspace)
adds cli option to the dynamic-sidecar to push the outputs ports
fixes dy-sidecar issue: sometimes containers are created but not started #3233

director-v2

all failing dynamic-sidecars will now be removed from the system (only the ones which had issues while saving data will still be kept and require manual intervention)

webserver

fixed an issue where the webserver was passing the wrong parameter to the director-v2 in the API when closing a service. It is now possible to remove nodes once more. This was no longer possible.

Related issue/s

How to test

Checklist

Unit tests for the changes exist
Runs in the swarm

…car-replace-long-running

mrnicegyu11

maybe consider adressing my requests, especially the process that is started explicitly without a termination timeout.

thanks for this

services/director-v2/src/simcore_service_director_v2/modules/dynamic_sidecar/scheduler/task.py

mrnicegyu11 · 2022-08-05T08:21:48Z

services/director-v2/src/simcore_service_director_v2/modules/dynamic_sidecar/scheduler/task.py

+                )
+                if failed_while_saving_state_and_outputs:
+                    # Since user data is important and must be saved, take no further
+                    # action and wait for manual intervention from support.


Please log a clear line here that can be used to create a graylog alert, like so:

logger.log("Dynamic sidecar SIDECARNAME of node uuid NODEUUID failed to save its data and needs manual intervention. This is a #SIMCORE_GRAYLOGALERT"

We could ake a mail alert for "#SIMCORE_GRAYLOGALERT"

@GitHK see the proposal I did for the same issue below. the graylog alert is a very good idea. @mrnicegyu11 can you define one, or that would be a ENV variable ?

I would advise against this one. Since this will be triggered every 5 seconds. You can pickup errors with sidecars if you try to search for OEC: in the logs. If you want that precise error. Search for
Could not contact dynamic-sidecar to save service state or upload outputs
Or maybe would you want me to replace that message with the above suggestion?

mrnicegyu11 · 2022-08-05T09:50:33Z

services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/core/docker_compose_utils.py

+    result = await _write_file_and_spawn_process(
+        compose_spec_yaml,
+        command=f'docker-compose --project-name {settings.DYNAMIC_SIDECAR_COMPOSE_NAMESPACE} --file "{{file_path}}" start',
+        process_termination_timeout=None,


I am quite sure we would want a termination timeout here, maybe of one hour or something high.

I'm still not sure how to deal with these. I will have a PR following this one with a review of all the timeouts.

mrnicegyu11 · 2022-08-05T09:51:04Z

services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/long_running_tasks.py

@@ -59,7 +60,8 @@ async def task_create_service_containers(
    )
    shared_store.container_names = assemble_container_names(shared_store.compose_spec)

-    logger.debug("Validated compose-spec:\n%s", f"{shared_store.compose_spec}")
+    # it is useful to see what compose spec was used to start the containers
+    logger.warning("Validated compose-spec:\n%s", f"{shared_store.compose_spec}")


This seems like an INFO, and thus the default loglevel should be info and this message should maybe be info

It is "info" but something a support person would like to see. This is only displayed to you if you log it at level warning.

I do agree with @mrnicegyu11 here. let's not write the code for how the log level is set in a specific deployment. it makes no sense to make it a warning. If you want to see info messages in the logs just set the log level to info. you can probably set something like DY_SIDECAR_LOG_LEVEL=info right @GitHK ?

Add "Info:" to the string of the log thx

what about this one?

That is not what I want. I want to have a warning level and also see this message. If I set the log level to DY_SIDECAR_LOG_LEVEL=INFO it gets too spammy.

sanderegg

looking very good, please have a look at my comments and we can discuss if something is unclear

services/director-v2/src/simcore_service_director_v2/modules/dynamic_sidecar/scheduler/task.py

sanderegg · 2022-08-05T09:14:03Z

services/director-v2/src/simcore_service_director_v2/modules/dynamic_sidecar/scheduler/task.py


-                # - cleanup: after removing?
+                logger.warning(
+                    "Removed %s service after error, no state saving was required.",


from the code above, I see that it will run this if cas_save is False, but also if were_services_created is False, where then this message is wrong.
And this brings the following question: Are you then saving the state if the services were not created? If yes why and is it necessary? Anyway you need to change the message...

It is correct for this to run if were_services_created is False or can_save is False. Both start from False values.
State is never saved if the services are not created.
I've tried to reword the message, but the meaning is the same for me, since it is what happens.

sanderegg · 2022-08-05T09:17:48Z

services/director-v2/tests/unit/test_modules_dynamic_sidecar_scheduler_task.py

@@ -132,14 +143,23 @@ async def test_skip_observation_cycle_after_error(
    scheduler: DynamicSidecarsScheduler,


what about creating a test per use case? just to be on the safe side
Like deleting the node, this should always work right?

OK. I've added all the possible cases. Please have a look at the tests.

services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/long_running_tasks.py

sanderegg

Cool! please have a look at the last comments.

services/director-v2/src/simcore_service_director_v2/modules/dynamic_sidecar/scheduler/task.py

services/director-v2/tests/unit/test_modules_dynamic_sidecar_scheduler_task.py

sanderegg · 2022-08-05T12:58:48Z

services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/long_running_tasks.py

@@ -59,7 +60,8 @@ async def task_create_service_containers(
    )
    shared_store.container_names = assemble_container_names(shared_store.compose_spec)

-    logger.debug("Validated compose-spec:\n%s", f"{shared_store.compose_spec}")
+    # it is useful to see what compose spec was used to start the containers
+    logger.warning("Validated compose-spec:\n%s", f"{shared_store.compose_spec}")


what about this one?

sonarqubecloud · 2022-08-05T13:37:38Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.3% Duplication

sanderegg

Can you please just check this last comment?

services/director-v2/src/simcore_service_director_v2/modules/dynamic_sidecar/scheduler/task.py

Andrei Neagu added 30 commits July 22, 2022 09:02

refactor long_running_tasks

0162fca

renamed routing

44e50d3

added docker-compose pull

61a1bcb

added create_service_containers_task with tests

5b1a5f4

Merge remote-tracking branch 'upstream/master' into pr-osparc-dy-side…

2e79229

…car-replace-long-running

codestyle

e394437

adds docker_compose down as a task

20dc145

added task_restore_state

873013f

add task to save state

40c9c48

added missing parameters

9addebb

refactored test to check uniquness of tasks

03319bc

Merge remote-tracking branch 'upstream/master' into pr-osparc-dy-side…

abbd521

…car-replace-long-running

moved tasks from api interface

8fb5c9a

renamed to public from private

3a8301d

fixed test

d0046fa

added task_ports_inputs_pull

9dcf919

Merge remote-tracking branch 'upstream/master' into pr-osparc-dy-side…

1f19018

…car-replace-long-running

added task_ports_outputs_pull and task_ports_outputs_push

e6d75ce

added containers_restart_task and update openapi.json

7886525

added missing test

1a26299

added interface to thin client

c298af7

extended public interface

f3525d0

pylint

c6499c9

replaced blocking start_service_creation

c46dc7f

start_service_creation event call replaced

5b785d9

replaced longrunning begin_service_destruction

4eb93ee

remove unused

55d3169

task_id is also injected in the progres handler

d7ad9d0

added supoport for multiple tasks gathering in context manger

951fcfe

refactor to use new progress callable

9f90423

GitHK marked this pull request as ready for review August 5, 2022 08:15

GitHK requested review from pcrespov and sanderegg as code owners August 5, 2022 08:15

GitHK requested review from mrnicegyu11 and odeimaiz August 5, 2022 08:15

Andrei Neagu added 3 commits August 5, 2022 10:29

fixed flky test

47f8c8d

fixed broken test

04aeb01

adding time for the proxy to start

51a3419

mrnicegyu11 approved these changes Aug 5, 2022

View reviewed changes

sanderegg reviewed Aug 5, 2022

View reviewed changes

Andrei Neagu added 5 commits August 5, 2022 13:30

reverted change

1877642

renaming to make it more coherent

cc57965

refactor wording

fe8f369

expanded with all possible use cases and outcomes

f4870aa

fixed logger policy

e758df5

GitHK requested review from sanderegg and mrnicegyu11 August 5, 2022 12:21

mrnicegyu11 changed the title ~~✨🐛 Follow-up removing long running http requests from dy-sidecar~~ ✨🐛 Follow-up removing long running http requests from dy-sidecar (DEVOPS!) Aug 5, 2022

mrnicegyu11 changed the title ~~✨🐛 Follow-up removing long running http requests from dy-sidecar (DEVOPS!)~~ ✨🐛 Follow-up removing long running http requests from dy-sidecar (🚧 DEVOPS!) Aug 5, 2022

refactor comments

09a3939

sanderegg approved these changes Aug 5, 2022

View reviewed changes

fixed failing tests

5a7b1be

GitHK changed the title ~~✨🐛 Follow-up removing long running http requests from dy-sidecar (🚧 DEVOPS!)~~ ✨🐛 Follow-up removing long running http requests from dy-sidecar (⚠️ devops) Aug 5, 2022

finally removing flakyness in tests

6db1e66

sanderegg approved these changes Aug 8, 2022

View reviewed changes

services/director-v2/src/simcore_service_director_v2/modules/dynamic_sidecar/scheduler/task.py Show resolved Hide resolved

sanderegg merged commit 4a0057e into ITISFoundation:master Aug 8, 2022

GitHK deleted the pr-osparc-followup-long-running branch August 8, 2022 06:53

GitHK added the changelog:⚠️devops label Aug 8, 2022

sanderegg mentioned this pull request Aug 27, 2022

improving dynamic-sidecar design ITISFoundation/osparc-issues#638

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨🐛 Follow-up removing long running http requests from dy-sidecar (⚠️ devops) #3232

✨🐛 Follow-up removing long running http requests from dy-sidecar (⚠️ devops) #3232

GitHK commented Aug 3, 2022 •

edited

Loading

mrnicegyu11 left a comment

mrnicegyu11 Aug 5, 2022

sanderegg Aug 5, 2022

GitHK Aug 5, 2022

mrnicegyu11 Aug 5, 2022

GitHK Aug 5, 2022

mrnicegyu11 Aug 5, 2022

GitHK Aug 5, 2022

sanderegg Aug 5, 2022

mrnicegyu11 Aug 5, 2022

sanderegg Aug 5, 2022

GitHK Aug 5, 2022

sanderegg left a comment

sanderegg Aug 5, 2022

GitHK Aug 5, 2022

sanderegg Aug 5, 2022

GitHK Aug 5, 2022

sanderegg left a comment

sanderegg Aug 5, 2022

sonarqubecloud bot commented Aug 5, 2022

sanderegg left a comment

		@@ -132,14 +143,23 @@ async def test_skip_observation_cycle_after_error(
		scheduler: DynamicSidecarsScheduler,

✨🐛 Follow-up removing long running http requests from dy-sidecar (⚠️ devops) #3232

✨🐛 Follow-up removing long running http requests from dy-sidecar (⚠️ devops) #3232

Conversation

GitHK commented Aug 3, 2022 • edited Loading

⚠️ devops

What do these changes do?

dynamic-sidecar

director-v2

webserver

Related issue/s

How to test

Checklist

mrnicegyu11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanderegg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanderegg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Aug 5, 2022

sanderegg left a comment

Choose a reason for hiding this comment

GitHK commented Aug 3, 2022 •

edited

Loading