| Message ID | 20201116152230.136349-1-sava.jakovljev@teufel.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Suricatta: Reset global state when cancelation request has been received | expand |
Hi Sava, On 16.11.20 16:22, Sava Jakovljev wrote: > Signed-off-by: Sava Jakovljev <sava.jakovljev@teufel.de> > --- > suricatta/server_hawkbit.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/suricatta/server_hawkbit.c b/suricatta/server_hawkbit.c > index db82ac8..a760049 100644 > --- a/suricatta/server_hawkbit.c > +++ b/suricatta/server_hawkbit.c > @@ -733,8 +733,17 @@ server_op_res_t server_has_pending_action(int *action_id) > } > if (result == SERVER_UPDATE_CANCELED) { > DEBUG("Acknowledging cancelled update."); > - (void)server_send_cancel_reply(server_hawkbit.channel, *action_id); > /* Inform the installer that a CANCEL was received */ > + (void)server_send_cancel_reply(server_hawkbit.channel, *action_id); > + > + server_hawkbit.update_state = STATE_OK; > + /* > + * Save the state > + */ > + if ((result = save_state((char *)STATE_KEY, STATE_OK)) != SERVER_OK) { > + ERROR("Error while resetting update state on persistent " > + "storage.\n"); > + } > return SERVER_OK; > } > I need to better understand the context. As far as I know, a "cancelAction" is provided by Hawkbit until this is not aknowledged. So it is true, the state is not updated here, but next time SWUpdate will connect again with the server and Hawkbit should not provide the URL for CancelUpdate, and this closes the whole transaction. State is not set explicitly by SWUpdate, but implicitly at next polling. And if a CancelAction is still present, Hawkbit has not closed the transaction and SWUpdate sends it again. So Hawkbit reset the state. Can you explain which bug should this patch fix, or which is the test case ? Best regards, Stefano Babic
Hello Stefano,
Problem I see is that when upgrade is installed, state is changed to
STATE_INSTALLED, and until the user doesn't reboot, we don't change it -
not even if at any point after installation (but before reboot) upgrade is
cancelled on Hawkbit.
This is a bit of an edge-case, but since SWUpdate is an upgrade framework,
I feel like enforcing or expecting that reboot will be done immediately
would be enforcing policy and that's not what frameworks do.
If my reasoning is correct: when we call server_has_pending_action,
subsequent call to server_get_deployment_info will return
SERVER_UPDATE_CANCELED. Next we acknowledge the cancellation and return
SERVER_OK, and exit - and there we're done. As a result, Hawkbit closes the
corresponding action. On the next poll, as a consequence,
server_get_deployment_info returns SERVER_NO_UPDATE_AVAILABLE, but our
state is still STATE_INSTALLED - no one did the reset to STATE_OK after
cancellation. As a consequence, assigning new action to the device results
in SWUpdate ignoring it - because of the condition:
if ((result == SERVER_UPDATE_AVAILABLE) &&
(get_state() == STATE_INSTALLED)) {
...
Please correct me if my logic is flawed.
server_get_deployment_info is not (and should not) manipulate the state,
nor handle_feedback, so we're good there - but extending has_pending_action
to cover this case also makes sense, as far as I see.
Also, doing it like suggested in this change is a little bit more clean,
since one reading the code has no illusions about what happens - we
explicitly reset the state after cancellation and that is clear, in my
opinion.
Of course, we can discuss this further, as I plan to push the patch that
would enable rejection of cancellation requests - but going one step at a
time, which would enable easier testing and tracking of potential problems.
Thank you.
Best regards,
Sava Jakovljev
Stefano Babic schrieb am Montag, 16. November 2020 um 16:56:10 UTC+1:
> Hi Sava,
>
> On 16.11.20 16:22, Sava Jakovljev wrote:
> > Signed-off-by: Sava Jakovljev <sava.ja...@teufel.de>
> > ---
> > suricatta/server_hawkbit.c | 11 ++++++++++-
> > 1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/suricatta/server_hawkbit.c b/suricatta/server_hawkbit.c
> > index db82ac8..a760049 100644
> > --- a/suricatta/server_hawkbit.c
> > +++ b/suricatta/server_hawkbit.c
> > @@ -733,8 +733,17 @@ server_op_res_t server_has_pending_action(int
> *action_id)
> > }
> > if (result == SERVER_UPDATE_CANCELED) {
> > DEBUG("Acknowledging cancelled update.");
> > - (void)server_send_cancel_reply(server_hawkbit.channel, *action_id);
> > /* Inform the installer that a CANCEL was received */
> > + (void)server_send_cancel_reply(server_hawkbit.channel, *action_id);
> > +
> > + server_hawkbit.update_state = STATE_OK;
> > + /*
> > + * Save the state
> > + */
> > + if ((result = save_state((char *)STATE_KEY, STATE_OK)) != SERVER_OK) {
> > + ERROR("Error while resetting update state on persistent "
> > + "storage.\n");
> > + }
> > return SERVER_OK;
> > }
> >
>
> I need to better understand the context. As far as I know, a
> "cancelAction" is provided by Hawkbit until this is not aknowledged. So
> it is true, the state is not updated here, but next time SWUpdate will
> connect again with the server and Hawkbit should not provide the URL for
> CancelUpdate, and this closes the whole transaction. State is not set
> explicitly by SWUpdate, but implicitly at next polling. And if a
> CancelAction is still present, Hawkbit has not closed the transaction
> and SWUpdate sends it again. So Hawkbit reset the state.
>
> Can you explain which bug should this patch fix, or which is the test case
> ?
>
> Best regards,
> Stefano Babic
>
> --
> =====================================================================
> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-53 <+49%208142%206698953> Fax: +49-8142-66989-80
> <+49%208142%206698980> Email: sba...@denx.de
> =====================================================================
>
Hi Sava, On 16.11.20 17:27, Sava Jakovljev wrote: > Hello Stefano, > > Problem I see is that when upgrade is installed, state is changed to > STATE_INSTALLED, Right. > and until the user doesn't reboot, we don't change it - > not even if at any point after installation (but before reboot) upgrade > is cancelled on Hawkbit. mmhhh..does it not change to STATE_FAILED or whatever ? ok, I understand now the issue. After sending the feedback for a CancelUpdate, state should change. > This is a bit of an edge-case, but since SWUpdate is an upgrade > framework, I feel like enforcing or expecting that reboot will be done > immediately would be enforcing policy and that's not what frameworks do. > > If my reasoning is correct: when we call server_has_pending_action, > subsequent call to server_get_deployment_info will return > SERVER_UPDATE_CANCELED. If Hawkbit has cancelled the update, yes. > Next we acknowledge the cancellation and return > SERVER_OK, and exit - and there we're done. I see, then STATE is still installed and no update will be fetch. Got it. > As a result, Hawkbit closes > the corresponding action. On the next poll, as a consequence, > server_get_deployment_info returns SERVER_NO_UPDATE_AVAILABLE, but our > state is still STATE_INSTALLED Worse is when Hawkbit have a new update, SWUpdate won't download it. > - no one did the reset to STATE_OK after > cancellation. As a consequence, assigning new action to the device > results in SWUpdate ignoring it - because of the condition: > if ((result == SERVER_UPDATE_AVAILABLE) && > (get_state() == STATE_INSTALLED)) { > ... Ok, fine with me. > > Please correct me if my logic is flawed. > server_get_deployment_info is not (and should not) manipulate the state, > nor handle_feedback, so we're good there - but extending > has_pending_action to cover this case also makes sense, as far as I see. Yes, it is - I will apply the patch. Best regards, Stefano Babic > Also, doing it like suggested in this change is a little bit more clean, > since one reading the code has no illusions about what happens - we > explicitly reset the state after cancellation and that is clear, in my > opinion. > Of course, we can discuss this further, as I plan to push the patch that > would enable rejection of cancellation requests - but going one step at > a time, which would enable easier testing and tracking of potential > problems. > > Thank you. > Best regards, > Sava Jakovljev > > Stefano Babic schrieb am Montag, 16. November 2020 um 16:56:10 UTC+1: > > Hi Sava, > > On 16.11.20 16:22, Sava Jakovljev wrote: > > Signed-off-by: Sava Jakovljev <sava.ja...@teufel.de> > > --- > > suricatta/server_hawkbit.c | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/suricatta/server_hawkbit.c b/suricatta/server_hawkbit.c > > index db82ac8..a760049 100644 > > --- a/suricatta/server_hawkbit.c > > +++ b/suricatta/server_hawkbit.c > > @@ -733,8 +733,17 @@ server_op_res_t server_has_pending_action(int > *action_id) > > } > > if (result == SERVER_UPDATE_CANCELED) { > > DEBUG("Acknowledging cancelled update."); > > - (void)server_send_cancel_reply(server_hawkbit.channel, *action_id); > > /* Inform the installer that a CANCEL was received */ > > + (void)server_send_cancel_reply(server_hawkbit.channel, *action_id); > > + > > + server_hawkbit.update_state = STATE_OK; > > + /* > > + * Save the state > > + */ > > + if ((result = save_state((char *)STATE_KEY, STATE_OK)) != > SERVER_OK) { > > + ERROR("Error while resetting update state on persistent " > > + "storage.\n"); > > + } > > return SERVER_OK; > > } > > > > I need to better understand the context. As far as I know, a > "cancelAction" is provided by Hawkbit until this is not aknowledged. So > it is true, the state is not updated here, but next time SWUpdate will > connect again with the server and Hawkbit should not provide the URL > for > CancelUpdate, and this closes the whole transaction. State is not set > explicitly by SWUpdate, but implicitly at next polling. And if a > CancelAction is still present, Hawkbit has not closed the transaction > and SWUpdate sends it again. So Hawkbit reset the state. > > Can you explain which bug should this patch fix, or which is the > test case ? > > Best regards, > Stefano Babic > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-53 <tel:+49%208142%206698953> Fax: > +49-8142-66989-80 <tel:+49%208142%206698980> Email: sba...@denx.de > ===================================================================== > > -- > You received this message because you are subscribed to the Google > Groups "swupdate" group. > To unsubscribe from this group and stop receiving emails from it, send > an email to swupdate+unsubscribe@googlegroups.com > <mailto:swupdate+unsubscribe@googlegroups.com>. > To view this discussion on the web visit > https://groups.google.com/d/msgid/swupdate/2709d14f-fdb8-4f40-a82d-040a5118dbf7n%40googlegroups.com > <https://groups.google.com/d/msgid/swupdate/2709d14f-fdb8-4f40-a82d-040a5118dbf7n%40googlegroups.com?utm_medium=email&utm_source=footer>.
Hi Stefano, Sorry for disturbance - any update on this? Thank you. Best regards, Sava Jakovljev Stefano Babic schrieb am Montag, 16. November 2020 um 17:52:45 UTC+1: > Hi Sava, > > On 16.11.20 17:27, Sava Jakovljev wrote: > > Hello Stefano, > > > > Problem I see is that when upgrade is installed, state is changed to > > STATE_INSTALLED, > > Right. > > > and until the user doesn't reboot, we don't change it - > > not even if at any point after installation (but before reboot) upgrade > > is cancelled on Hawkbit. > > mmhhh..does it not change to STATE_FAILED or whatever ? ok, I understand > now the issue. After sending the feedback for a CancelUpdate, state > should change. > > > This is a bit of an edge-case, but since SWUpdate is an upgrade > > framework, I feel like enforcing or expecting that reboot will be done > > immediately would be enforcing policy and that's not what frameworks do. > > > > If my reasoning is correct: when we call server_has_pending_action, > > subsequent call to server_get_deployment_info will return > > SERVER_UPDATE_CANCELED. > > If Hawkbit has cancelled the update, yes. > > > Next we acknowledge the cancellation and return > > SERVER_OK, and exit - and there we're done. > > I see, then STATE is still installed and no update will be fetch. Got it. > > > As a result, Hawkbit closes > > the corresponding action. On the next poll, as a consequence, > > server_get_deployment_info returns SERVER_NO_UPDATE_AVAILABLE, but our > > state is still STATE_INSTALLED > > Worse is when Hawkbit have a new update, SWUpdate won't download it. > > > - no one did the reset to STATE_OK after > > cancellation. As a consequence, assigning new action to the device > > results in SWUpdate ignoring it - because of the condition: > > if ((result == SERVER_UPDATE_AVAILABLE) && > > (get_state() == STATE_INSTALLED)) { > > ... > > Ok, fine with me. > > > > > Please correct me if my logic is flawed. > > server_get_deployment_info is not (and should not) manipulate the state, > > nor handle_feedback, so we're good there - but extending > > has_pending_action to cover this case also makes sense, as far as I see. > > Yes, it is - I will apply the patch. > > Best regards, > Stefano Babic > > > Also, doing it like suggested in this change is a little bit more clean, > > since one reading the code has no illusions about what happens - we > > explicitly reset the state after cancellation and that is clear, in my > > opinion. > > Of course, we can discuss this further, as I plan to push the patch that > > would enable rejection of cancellation requests - but going one step at > > a time, which would enable easier testing and tracking of potential > > problems. > > > > Thank you. > > Best regards, > > Sava Jakovljev > > > > Stefano Babic schrieb am Montag, 16. November 2020 um 16:56:10 UTC+1: > > > > Hi Sava, > > > > On 16.11.20 16:22, Sava Jakovljev wrote: > > > Signed-off-by: Sava Jakovljev <sava.ja...@teufel.de> > > > --- > > > suricatta/server_hawkbit.c | 11 ++++++++++- > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > diff --git a/suricatta/server_hawkbit.c b/suricatta/server_hawkbit.c > > > index db82ac8..a760049 100644 > > > --- a/suricatta/server_hawkbit.c > > > +++ b/suricatta/server_hawkbit.c > > > @@ -733,8 +733,17 @@ server_op_res_t server_has_pending_action(int > > *action_id) > > > } > > > if (result == SERVER_UPDATE_CANCELED) { > > > DEBUG("Acknowledging cancelled update."); > > > - (void)server_send_cancel_reply(server_hawkbit.channel, *action_id); > > > /* Inform the installer that a CANCEL was received */ > > > + (void)server_send_cancel_reply(server_hawkbit.channel, *action_id); > > > + > > > + server_hawkbit.update_state = STATE_OK; > > > + /* > > > + * Save the state > > > + */ > > > + if ((result = save_state((char *)STATE_KEY, STATE_OK)) != > > SERVER_OK) { > > > + ERROR("Error while resetting update state on persistent " > > > + "storage.\n"); > > > + } > > > return SERVER_OK; > > > } > > > > > > > I need to better understand the context. As far as I know, a > > "cancelAction" is provided by Hawkbit until this is not aknowledged. So > > it is true, the state is not updated here, but next time SWUpdate will > > connect again with the server and Hawkbit should not provide the URL > > for > > CancelUpdate, and this closes the whole transaction. State is not set > > explicitly by SWUpdate, but implicitly at next polling. And if a > > CancelAction is still present, Hawkbit has not closed the transaction > > and SWUpdate sends it again. So Hawkbit reset the state. > > > > Can you explain which bug should this patch fix, or which is the > > test case ? > > > > Best regards, > > Stefano Babic > > > > -- > > ===================================================================== > > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > > Phone: +49-8142-66989-53 <+49%208142%206698953> > <tel:+49%208142%206698953> Fax: > > +49-8142-66989-80 <+49%208142%206698980> <tel:+49%208142%206698980> > Email: sba...@denx.de > > ===================================================================== > > > > -- > > You received this message because you are subscribed to the Google > > Groups "swupdate" group. > > To unsubscribe from this group and stop receiving emails from it, send > > an email to swupdate+u...@googlegroups.com > > <mailto:swupdate+u...@googlegroups.com>. > > To view this discussion on the web visit > > > https://groups.google.com/d/msgid/swupdate/2709d14f-fdb8-4f40-a82d-040a5118dbf7n%40googlegroups.com > > < > https://groups.google.com/d/msgid/swupdate/2709d14f-fdb8-4f40-a82d-040a5118dbf7n%40googlegroups.com?utm_medium=email&utm_source=footer > >. > > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-53 <+49%208142%206698953> Fax: +49-8142-66989-80 > <+49%208142%206698980> Email: sba...@denx.de > ===================================================================== >
Hi Sava, On 25.11.20 18:20, Sava Jakovljev wrote: > Hi Stefano, > > Sorry for disturbance - any update on this? > No disturb - thanks for remind. I had already set the patch as "Accepted" in patchwork before applying, and then it was not anymore in my queue. It is in. Regards, Stefano > Thank you. > Best regards, > Sava Jakovljev > Stefano Babic schrieb am Montag, 16. November 2020 um 17:52:45 UTC+1: > > Hi Sava, > > On 16.11.20 17:27, Sava Jakovljev wrote: > > Hello Stefano, > > > > Problem I see is that when upgrade is installed, state is changed to > > STATE_INSTALLED, > > Right. > > > and until the user doesn't reboot, we don't change it - > > not even if at any point after installation (but before reboot) > upgrade > > is cancelled on Hawkbit. > > mmhhh..does it not change to STATE_FAILED or whatever ? ok, I > understand > now the issue. After sending the feedback for a CancelUpdate, state > should change. > > > This is a bit of an edge-case, but since SWUpdate is an upgrade > > framework, I feel like enforcing or expecting that reboot will be > done > > immediately would be enforcing policy and that's not what > frameworks do. > > > > If my reasoning is correct: when we call server_has_pending_action, > > subsequent call to server_get_deployment_info will return > > SERVER_UPDATE_CANCELED. > > If Hawkbit has cancelled the update, yes. > > > Next we acknowledge the cancellation and return > > SERVER_OK, and exit - and there we're done. > > I see, then STATE is still installed and no update will be fetch. > Got it. > > > As a result, Hawkbit closes > > the corresponding action. On the next poll, as a consequence, > > server_get_deployment_info returns SERVER_NO_UPDATE_AVAILABLE, > but our > > state is still STATE_INSTALLED > > Worse is when Hawkbit have a new update, SWUpdate won't download it. > > > - no one did the reset to STATE_OK after > > cancellation. As a consequence, assigning new action to the device > > results in SWUpdate ignoring it - because of the condition: > > if ((result == SERVER_UPDATE_AVAILABLE) && > > (get_state() == STATE_INSTALLED)) { > > ... > > Ok, fine with me. > > > > > Please correct me if my logic is flawed. > > server_get_deployment_info is not (and should not) manipulate the > state, > > nor handle_feedback, so we're good there - but extending > > has_pending_action to cover this case also makes sense, as far as > I see. > > Yes, it is - I will apply the patch. > > Best regards, > Stefano Babic > > > Also, doing it like suggested in this change is a little bit more > clean, > > since one reading the code has no illusions about what happens - we > > explicitly reset the state after cancellation and that is clear, > in my > > opinion. > > Of course, we can discuss this further, as I plan to push the > patch that > > would enable rejection of cancellation requests - but going one > step at > > a time, which would enable easier testing and tracking of potential > > problems. > > > > Thank you. > > Best regards, > > Sava Jakovljev > > > > Stefano Babic schrieb am Montag, 16. November 2020 um 16:56:10 > UTC+1: > > > > Hi Sava, > > > > On 16.11.20 16:22, Sava Jakovljev wrote: > > > Signed-off-by: Sava Jakovljev <sava.ja...@teufel.de> > > > --- > > > suricatta/server_hawkbit.c | 11 ++++++++++- > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > diff --git a/suricatta/server_hawkbit.c > b/suricatta/server_hawkbit.c > > > index db82ac8..a760049 100644 > > > --- a/suricatta/server_hawkbit.c > > > +++ b/suricatta/server_hawkbit.c > > > @@ -733,8 +733,17 @@ server_op_res_t server_has_pending_action(int > > *action_id) > > > } > > > if (result == SERVER_UPDATE_CANCELED) { > > > DEBUG("Acknowledging cancelled update."); > > > - (void)server_send_cancel_reply(server_hawkbit.channel, > *action_id); > > > /* Inform the installer that a CANCEL was received */ > > > + (void)server_send_cancel_reply(server_hawkbit.channel, > *action_id); > > > + > > > + server_hawkbit.update_state = STATE_OK; > > > + /* > > > + * Save the state > > > + */ > > > + if ((result = save_state((char *)STATE_KEY, STATE_OK)) != > > SERVER_OK) { > > > + ERROR("Error while resetting update state on persistent " > > > + "storage.\n"); > > > + } > > > return SERVER_OK; > > > } > > > > > > > I need to better understand the context. As far as I know, a > > "cancelAction" is provided by Hawkbit until this is not > aknowledged. So > > it is true, the state is not updated here, but next time SWUpdate > will > > connect again with the server and Hawkbit should not provide the URL > > for > > CancelUpdate, and this closes the whole transaction. State is not > set > > explicitly by SWUpdate, but implicitly at next polling. And if a > > CancelAction is still present, Hawkbit has not closed the > transaction > > and SWUpdate sends it again. So Hawkbit reset the state. > > > > Can you explain which bug should this patch fix, or which is the > > test case ? > > > > Best regards, > > Stefano Babic > > > > -- > > > ===================================================================== > > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, > Germany > > Phone: +49-8142-66989-53 <tel:+49%208142%206698953> > <tel:+49%208142%206698953> Fax: > > +49-8142-66989-80 <tel:+49%208142%206698980> > <tel:+49%208142%206698980> Email: sba...@denx.de > > > ===================================================================== > > > > -- > > You received this message because you are subscribed to the Google > > Groups "swupdate" group. > > To unsubscribe from this group and stop receiving emails from it, > send > > an email to swupdate+u...@googlegroups.com > > <mailto:swupdate+u...@googlegroups.com>. > > To view this discussion on the web visit > > > https://groups.google.com/d/msgid/swupdate/2709d14f-fdb8-4f40-a82d-040a5118dbf7n%40googlegroups.com > > > > <https://groups.google.com/d/msgid/swupdate/2709d14f-fdb8-4f40-a82d-040a5118dbf7n%40googlegroups.com?utm_medium=email&utm_source=footer>. > > > > -- > ===================================================================== > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-53 <tel:+49%208142%206698953> Fax: > +49-8142-66989-80 <tel:+49%208142%206698980> Email: sba...@denx.de > ===================================================================== > > -- > You received this message because you are subscribed to the Google > Groups "swupdate" group. > To unsubscribe from this group and stop receiving emails from it, send > an email to swupdate+unsubscribe@googlegroups.com > <mailto:swupdate+unsubscribe@googlegroups.com>. > To view this discussion on the web visit > https://groups.google.com/d/msgid/swupdate/d4fd31c3-ea35-4814-ad20-82a2d047d92bn%40googlegroups.com > <https://groups.google.com/d/msgid/swupdate/d4fd31c3-ea35-4814-ad20-82a2d047d92bn%40googlegroups.com?utm_medium=email&utm_source=footer>.
Hi Sava, On 28.11.20 13:12, Stefano Babic wrote: > Hi Sava, > > On 25.11.20 18:20, Sava Jakovljev wrote: >> Hi Stefano, >> >> Sorry for disturbance - any update on this? >> > > No disturb - thanks for remind. I had already set the patch as > "Accepted" in patchwork before applying, and then it was not anymore in > my queue. It is in. > Mmhh.. I get build error when I run "make test". Log is here: https://travis-ci.org/github/sbabic/swupdate/builds/746416522 Could you take a look ? Regards, Stefano > Regards, > Stefano > >> Thank you. >> Best regards, >> Sava Jakovljev >> Stefano Babic schrieb am Montag, 16. November 2020 um 17:52:45 UTC+1: >> >> Hi Sava, >> >> On 16.11.20 17:27, Sava Jakovljev wrote: >> > Hello Stefano, >> > >> > Problem I see is that when upgrade is installed, state is >> changed to >> > STATE_INSTALLED, >> >> Right. >> >> > and until the user doesn't reboot, we don't change it - >> > not even if at any point after installation (but before reboot) >> upgrade >> > is cancelled on Hawkbit. >> >> mmhhh..does it not change to STATE_FAILED or whatever ? ok, I >> understand >> now the issue. After sending the feedback for a CancelUpdate, state >> should change. >> >> > This is a bit of an edge-case, but since SWUpdate is an upgrade >> > framework, I feel like enforcing or expecting that reboot will be >> done >> > immediately would be enforcing policy and that's not what >> frameworks do. >> > >> > If my reasoning is correct: when we call >> server_has_pending_action, >> > subsequent call to server_get_deployment_info will return >> > SERVER_UPDATE_CANCELED. >> >> If Hawkbit has cancelled the update, yes. >> >> > Next we acknowledge the cancellation and return >> > SERVER_OK, and exit - and there we're done. >> >> I see, then STATE is still installed and no update will be fetch. >> Got it. >> >> > As a result, Hawkbit closes >> > the corresponding action. On the next poll, as a consequence, >> > server_get_deployment_info returns SERVER_NO_UPDATE_AVAILABLE, >> but our >> > state is still STATE_INSTALLED >> >> Worse is when Hawkbit have a new update, SWUpdate won't download it. >> >> > - no one did the reset to STATE_OK after >> > cancellation. As a consequence, assigning new action to the device >> > results in SWUpdate ignoring it - because of the condition: >> > if ((result == SERVER_UPDATE_AVAILABLE) && >> > (get_state() == STATE_INSTALLED)) { >> > ... >> >> Ok, fine with me. >> >> > >> > Please correct me if my logic is flawed. >> > server_get_deployment_info is not (and should not) manipulate the >> state, >> > nor handle_feedback, so we're good there - but extending >> > has_pending_action to cover this case also makes sense, as far as >> I see. >> >> Yes, it is - I will apply the patch. >> >> Best regards, >> Stefano Babic >> >> > Also, doing it like suggested in this change is a little bit more >> clean, >> > since one reading the code has no illusions about what happens >> - we >> > explicitly reset the state after cancellation and that is clear, >> in my >> > opinion. >> > Of course, we can discuss this further, as I plan to push the >> patch that >> > would enable rejection of cancellation requests - but going one >> step at >> > a time, which would enable easier testing and tracking of >> potential >> > problems. >> > >> > Thank you. >> > Best regards, >> > Sava Jakovljev >> > >> > Stefano Babic schrieb am Montag, 16. November 2020 um 16:56:10 >> UTC+1: >> > >> > Hi Sava, >> > >> > On 16.11.20 16:22, Sava Jakovljev wrote: >> > > Signed-off-by: Sava Jakovljev <sava.ja...@teufel.de> >> > > --- >> > > suricatta/server_hawkbit.c | 11 ++++++++++- >> > > 1 file changed, 10 insertions(+), 1 deletion(-) >> > > >> > > diff --git a/suricatta/server_hawkbit.c >> b/suricatta/server_hawkbit.c >> > > index db82ac8..a760049 100644 >> > > --- a/suricatta/server_hawkbit.c >> > > +++ b/suricatta/server_hawkbit.c >> > > @@ -733,8 +733,17 @@ server_op_res_t >> server_has_pending_action(int >> > *action_id) >> > > } >> > > if (result == SERVER_UPDATE_CANCELED) { >> > > DEBUG("Acknowledging cancelled update."); >> > > - (void)server_send_cancel_reply(server_hawkbit.channel, >> *action_id); >> > > /* Inform the installer that a CANCEL was received */ >> > > + (void)server_send_cancel_reply(server_hawkbit.channel, >> *action_id); >> > > + >> > > + server_hawkbit.update_state = STATE_OK; >> > > + /* >> > > + * Save the state >> > > + */ >> > > + if ((result = save_state((char *)STATE_KEY, STATE_OK)) != >> > SERVER_OK) { >> > > + ERROR("Error while resetting update state on persistent " >> > > + "storage.\n"); >> > > + } >> > > return SERVER_OK; >> > > } >> > > >> > >> > I need to better understand the context. As far as I know, a >> > "cancelAction" is provided by Hawkbit until this is not >> aknowledged. So >> > it is true, the state is not updated here, but next time SWUpdate >> will >> > connect again with the server and Hawkbit should not provide >> the URL >> > for >> > CancelUpdate, and this closes the whole transaction. State is not >> set >> > explicitly by SWUpdate, but implicitly at next polling. And if a >> > CancelAction is still present, Hawkbit has not closed the >> transaction >> > and SWUpdate sends it again. So Hawkbit reset the state. >> > >> > Can you explain which bug should this patch fix, or which is the >> > test case ? >> > >> > Best regards, >> > Stefano Babic >> > >> > -- >> > >> ===================================================================== >> > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk >> > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, >> Germany >> > Phone: +49-8142-66989-53 <tel:+49%208142%206698953> >> <tel:+49%208142%206698953> Fax: >> > +49-8142-66989-80 <tel:+49%208142%206698980> >> <tel:+49%208142%206698980> Email: sba...@denx.de >> > >> ===================================================================== >> > >> > -- >> > You received this message because you are subscribed to the Google >> > Groups "swupdate" group. >> > To unsubscribe from this group and stop receiving emails from it, >> send >> > an email to swupdate+u...@googlegroups.com >> > <mailto:swupdate+u...@googlegroups.com>. >> > To view this discussion on the web visit >> > >> >> https://groups.google.com/d/msgid/swupdate/2709d14f-fdb8-4f40-a82d-040a5118dbf7n%40googlegroups.com >> >> >> > >> >> <https://groups.google.com/d/msgid/swupdate/2709d14f-fdb8-4f40-a82d-040a5118dbf7n%40googlegroups.com?utm_medium=email&utm_source=footer>. >> >> >> >> >> -- >> ===================================================================== >> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk >> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany >> Phone: +49-8142-66989-53 <tel:+49%208142%206698953> Fax: >> +49-8142-66989-80 <tel:+49%208142%206698980> Email: sba...@denx.de >> ===================================================================== >> >> -- >> You received this message because you are subscribed to the Google >> Groups "swupdate" group. >> To unsubscribe from this group and stop receiving emails from it, send >> an email to swupdate+unsubscribe@googlegroups.com >> <mailto:swupdate+unsubscribe@googlegroups.com>. >> To view this discussion on the web visit >> https://groups.google.com/d/msgid/swupdate/d4fd31c3-ea35-4814-ad20-82a2d047d92bn%40googlegroups.com >> <https://groups.google.com/d/msgid/swupdate/d4fd31c3-ea35-4814-ad20-82a2d047d92bn%40googlegroups.com?utm_medium=email&utm_source=footer>. >> >
On 28.11.20 13:42, Stefano Babic wrote: > Hi Sava, > > On 28.11.20 13:12, Stefano Babic wrote: >> Hi Sava, >> >> On 25.11.20 18:20, Sava Jakovljev wrote: >>> Hi Stefano, >>> >>> Sorry for disturbance - any update on this? >>> >> >> No disturb - thanks for remind. I had already set the patch as >> "Accepted" in patchwork before applying, and then it was not anymore >> in my queue. It is in. >> > > Mmhh.. I get build error when I run "make test". Log is here: > > https://travis-ci.org/github/sbabic/swupdate/builds/746416522 > > Could you take a look ? It does not matter, patch on the way. Regards, Stefano > > Regards, > Stefano > > >> Regards, >> Stefano >> >>> Thank you. >>> Best regards, >>> Sava Jakovljev >>> Stefano Babic schrieb am Montag, 16. November 2020 um 17:52:45 UTC+1: >>> >>> Hi Sava, >>> >>> On 16.11.20 17:27, Sava Jakovljev wrote: >>> > Hello Stefano, >>> > >>> > Problem I see is that when upgrade is installed, state is >>> changed to >>> > STATE_INSTALLED, >>> >>> Right. >>> >>> > and until the user doesn't reboot, we don't change it - >>> > not even if at any point after installation (but before reboot) >>> upgrade >>> > is cancelled on Hawkbit. >>> >>> mmhhh..does it not change to STATE_FAILED or whatever ? ok, I >>> understand >>> now the issue. After sending the feedback for a CancelUpdate, state >>> should change. >>> >>> > This is a bit of an edge-case, but since SWUpdate is an upgrade >>> > framework, I feel like enforcing or expecting that reboot will be >>> done >>> > immediately would be enforcing policy and that's not what >>> frameworks do. >>> > >>> > If my reasoning is correct: when we call >>> server_has_pending_action, >>> > subsequent call to server_get_deployment_info will return >>> > SERVER_UPDATE_CANCELED. >>> >>> If Hawkbit has cancelled the update, yes. >>> >>> > Next we acknowledge the cancellation and return >>> > SERVER_OK, and exit - and there we're done. >>> >>> I see, then STATE is still installed and no update will be fetch. >>> Got it. >>> >>> > As a result, Hawkbit closes >>> > the corresponding action. On the next poll, as a consequence, >>> > server_get_deployment_info returns SERVER_NO_UPDATE_AVAILABLE, >>> but our >>> > state is still STATE_INSTALLED >>> >>> Worse is when Hawkbit have a new update, SWUpdate won't download it. >>> >>> > - no one did the reset to STATE_OK after >>> > cancellation. As a consequence, assigning new action to the >>> device >>> > results in SWUpdate ignoring it - because of the condition: >>> > if ((result == SERVER_UPDATE_AVAILABLE) && >>> > (get_state() == STATE_INSTALLED)) { >>> > ... >>> >>> Ok, fine with me. >>> >>> > >>> > Please correct me if my logic is flawed. >>> > server_get_deployment_info is not (and should not) manipulate the >>> state, >>> > nor handle_feedback, so we're good there - but extending >>> > has_pending_action to cover this case also makes sense, as far as >>> I see. >>> >>> Yes, it is - I will apply the patch. >>> >>> Best regards, >>> Stefano Babic >>> >>> > Also, doing it like suggested in this change is a little bit more >>> clean, >>> > since one reading the code has no illusions about what happens >>> - we >>> > explicitly reset the state after cancellation and that is clear, >>> in my >>> > opinion. >>> > Of course, we can discuss this further, as I plan to push the >>> patch that >>> > would enable rejection of cancellation requests - but going one >>> step at >>> > a time, which would enable easier testing and tracking of >>> potential >>> > problems. >>> > >>> > Thank you. >>> > Best regards, >>> > Sava Jakovljev >>> > >>> > Stefano Babic schrieb am Montag, 16. November 2020 um 16:56:10 >>> UTC+1: >>> > >>> > Hi Sava, >>> > >>> > On 16.11.20 16:22, Sava Jakovljev wrote: >>> > > Signed-off-by: Sava Jakovljev <sava.ja...@teufel.de> >>> > > --- >>> > > suricatta/server_hawkbit.c | 11 ++++++++++- >>> > > 1 file changed, 10 insertions(+), 1 deletion(-) >>> > > >>> > > diff --git a/suricatta/server_hawkbit.c >>> b/suricatta/server_hawkbit.c >>> > > index db82ac8..a760049 100644 >>> > > --- a/suricatta/server_hawkbit.c >>> > > +++ b/suricatta/server_hawkbit.c >>> > > @@ -733,8 +733,17 @@ server_op_res_t >>> server_has_pending_action(int >>> > *action_id) >>> > > } >>> > > if (result == SERVER_UPDATE_CANCELED) { >>> > > DEBUG("Acknowledging cancelled update."); >>> > > - (void)server_send_cancel_reply(server_hawkbit.channel, >>> *action_id); >>> > > /* Inform the installer that a CANCEL was received */ >>> > > + (void)server_send_cancel_reply(server_hawkbit.channel, >>> *action_id); >>> > > + >>> > > + server_hawkbit.update_state = STATE_OK; >>> > > + /* >>> > > + * Save the state >>> > > + */ >>> > > + if ((result = save_state((char *)STATE_KEY, STATE_OK)) != >>> > SERVER_OK) { >>> > > + ERROR("Error while resetting update state on persistent " >>> > > + "storage.\n"); >>> > > + } >>> > > return SERVER_OK; >>> > > } >>> > > >>> > >>> > I need to better understand the context. As far as I know, a >>> > "cancelAction" is provided by Hawkbit until this is not >>> aknowledged. So >>> > it is true, the state is not updated here, but next time SWUpdate >>> will >>> > connect again with the server and Hawkbit should not provide >>> the URL >>> > for >>> > CancelUpdate, and this closes the whole transaction. State is not >>> set >>> > explicitly by SWUpdate, but implicitly at next polling. And if a >>> > CancelAction is still present, Hawkbit has not closed the >>> transaction >>> > and SWUpdate sends it again. So Hawkbit reset the state. >>> > >>> > Can you explain which bug should this patch fix, or which is the >>> > test case ? >>> > >>> > Best regards, >>> > Stefano Babic >>> > >>> > -- >>> > >>> >>> ===================================================================== >>> > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk >>> > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, >>> Germany >>> > Phone: +49-8142-66989-53 <tel:+49%208142%206698953> >>> <tel:+49%208142%206698953> Fax: >>> > +49-8142-66989-80 <tel:+49%208142%206698980> >>> <tel:+49%208142%206698980> Email: sba...@denx.de >>> > >>> >>> ===================================================================== >>> > >>> > -- >>> > You received this message because you are subscribed to the >>> Google >>> > Groups "swupdate" group. >>> > To unsubscribe from this group and stop receiving emails from it, >>> send >>> > an email to swupdate+u...@googlegroups.com >>> > <mailto:swupdate+u...@googlegroups.com>. >>> > To view this discussion on the web visit >>> > >>> https://groups.google.com/d/msgid/swupdate/2709d14f-fdb8-4f40-a82d-040a5118dbf7n%40googlegroups.com >>> >>> >>> > >>> <https://groups.google.com/d/msgid/swupdate/2709d14f-fdb8-4f40-a82d-040a5118dbf7n%40googlegroups.com?utm_medium=email&utm_source=footer>. >>> >>> >>> >>> >>> -- >>> ===================================================================== >>> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk >>> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, >>> Germany >>> Phone: +49-8142-66989-53 <tel:+49%208142%206698953> Fax: >>> +49-8142-66989-80 <tel:+49%208142%206698980> Email: sba...@denx.de >>> >>> ===================================================================== >>> >>> -- >>> You received this message because you are subscribed to the Google >>> Groups "swupdate" group. >>> To unsubscribe from this group and stop receiving emails from it, >>> send an email to swupdate+unsubscribe@googlegroups.com >>> <mailto:swupdate+unsubscribe@googlegroups.com>. >>> To view this discussion on the web visit >>> https://groups.google.com/d/msgid/swupdate/d4fd31c3-ea35-4814-ad20-82a2d047d92bn%40googlegroups.com >>> <https://groups.google.com/d/msgid/swupdate/d4fd31c3-ea35-4814-ad20-82a2d047d92bn%40googlegroups.com?utm_medium=email&utm_source=footer>. >>> >> >
diff --git a/suricatta/server_hawkbit.c b/suricatta/server_hawkbit.c index db82ac8..a760049 100644 --- a/suricatta/server_hawkbit.c +++ b/suricatta/server_hawkbit.c @@ -733,8 +733,17 @@ server_op_res_t server_has_pending_action(int *action_id) } if (result == SERVER_UPDATE_CANCELED) { DEBUG("Acknowledging cancelled update."); - (void)server_send_cancel_reply(server_hawkbit.channel, *action_id); /* Inform the installer that a CANCEL was received */ + (void)server_send_cancel_reply(server_hawkbit.channel, *action_id); + + server_hawkbit.update_state = STATE_OK; + /* + * Save the state + */ + if ((result = save_state((char *)STATE_KEY, STATE_OK)) != SERVER_OK) { + ERROR("Error while resetting update state on persistent " + "storage.\n"); + } return SERVER_OK; }
Signed-off-by: Sava Jakovljev <sava.jakovljev@teufel.de> --- suricatta/server_hawkbit.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)