[devel] [PATCH 0/1] Review Request for mds: Using timer to continue sending queued messages [#3127] V2
Summary: mds: Using timer to continue sending queued messages [#3127] Review request for Ticket(s): 3127 Peer Reviewer(s): Minh, Vu, Thang, Gary Pull request to: *** LIST THE PERSON WITH PUSH ACCESS HERE *** Affected branch(es): develop Development branch: ticket-3127 Base revision: 09eec5a1e422fec2cb97001fafb801b0b84529a1 Personal repository: git://git.code.sf.net/u/thuantr/review Impacted area Impact y/n Docsn Build systemn RPM/packaging n Configuration files n Startup scripts n SAF servicesn OpenSAF servicesn Core libraries y Samples n Tests n Other n Comments (indicate scope for each "y" above): - N/A revision 6a9481a0c76ebfd8ab433d924fab17eaae724af1 Author: thuan.tran Date: Tue, 10 Dec 2019 12:48:14 +0700 mds: Using timer to continue sending queued messages [#3127] - In overflow, receive chunk ack may stuck in retrying to send pending messages then later chunk ack comming cannot proceed. - Instead of retrying to send pending messages, reuse timer send chunk ack to trigger send pending messages if any. By this, even no more Nack or ChunkAck event comming, pending messages will be sent by timer. Complete diffstat: -- src/mds/mds_dt_tipc.c| 12 ++--- src/mds/mds_tipc_fctrl_intf.cc | 12 - src/mds/mds_tipc_fctrl_portid.cc | 95 +++- src/mds/mds_tipc_fctrl_portid.h | 2 + 4 files changed, 64 insertions(+), 57 deletions(-) Testing Commands: - N/A Testing, Expected Results: -- N/A Conditions of Submission: - ACK by reviewers Arch Built StartedLinux distro --- mipsn n mips64 n n x86 n n x86_64 y y powerpc n n powerpc64 n n Reviewer Checklist: --- [Submitters: make sure that your review doesn't trigger any checkmarks!] Your checkin has not passed review because (see checked entries): ___ Your RR template is generally incomplete; it has too many blank entries that need proper data filled in. ___ You have failed to nominate the proper persons for review and push. ___ Your patches do not have proper short+long header ___ You have grammar/spelling in your header that is unacceptable. ___ You have exceeded a sensible line length in your headers/comments/text. ___ You have failed to put in a proper Trac Ticket # into your commits. ___ You have incorrectly put/left internal data in your comments/files (i.e. internal bug tracking tool IDs, product names etc) ___ You have not given any evidence of testing beyond basic build tests. Demonstrate some level of runtime or other sanity testing. ___ You have ^M present in some of your files. These have to be removed. ___ You have needlessly changed whitespace or added whitespace crimes like trailing spaces, or spaces before tabs. ___ You have mixed real technical changes with whitespace and other cosmetic code cleanup changes. These have to be separate commits. ___ You need to refactor your submission into logical chunks; there is too much content into a single commit. ___ You have extraneous garbage in your review (merge commits etc) ___ You have giant attachments which should never have been sent; Instead you should place your content in a public tree to be pulled. ___ You have too many commits attached to an e-mail; resend as threaded commits, or place in a public tree for a pull. ___ You have resent this content multiple times without a clear indication of what has changed between each re-send. ___ You have failed to adequately and individually address all of the comments and change requests that were proposed in the initial review. ___ You have a misconfigured ~/.gitconfig file (i.e. user.name, user.email etc) ___ Your computer have a badly configured date and time; confusing the the threaded patch review. ___ Your changes affect IPC mechanism, and you don't present any results for in-service upgradability test. ___ Your changes affect user manual and documentation, your patch series do not contain the patch that updates the Doxygen manual. ___ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
Re: [devel] [PATCH 1/1] mds: fix ckpt 20 11 failure [#3127]
Best Regards,
ThuanTr
-Original Message-
From: Minh Hon Chau
Sent: Tuesday, December 10, 2019 9:51 AM
To: thuan.tran ; 'Nguyen Minh Vu'
; 'thang . d . nguyen'
; gary@dektech.com.au
Cc: opensaf-devel@lists.sourceforge.net
Subject: Re: [PATCH 1/1] mds: fix ckpt 20 11 failure [#3127]
Hi Thuan,
- We could give the patch title a bit more meanings than "fix ckpt 20
11..", for example, something as "Using timer to continue sending queued
message".
[Thuan] OK, will update short commit message.
- And a few comments inline
Thanks
Minh
On 5/12/19 3:05 pm, thuan.tran wrote:
> - In overflow, receive chunk ack may stuck in retrying to send pending
> messages then later chunk ack comming cannot proceed.
> - Instead of retrying to send pending messages, reuse timer send chunk
> ack to trigger send pending messages if any. By this, even no more Nack
> or ChunkAck event comming, pending messages will be sent by timer.
> ---
> src/mds/mds_dt_tipc.c| 12 ++---
> src/mds/mds_tipc_fctrl_intf.cc | 10
> src/mds/mds_tipc_fctrl_portid.cc | 88 ++--
> src/mds/mds_tipc_fctrl_portid.h | 1 +
> 4 files changed, 56 insertions(+), 55 deletions(-)
>
> diff --git a/src/mds/mds_dt_tipc.c b/src/mds/mds_dt_tipc.c
> index 9b3290833..6b30846a1 100644
> --- a/src/mds/mds_dt_tipc.c
> +++ b/src/mds/mds_dt_tipc.c
> @@ -3183,13 +3183,13 @@ ssize_t mds_retry_sendto(int sockfd, const void *buf,
> size_t len, int flags,
> {
> int retry = 5;
> ssize_t send_len = 0;
> - while (retry >= 0) {
> + while (retry-- >= 0) {
> send_len = sendto(sockfd, buf, len, flags, dest_addr, addrlen);
> if (send_len == len) {
> return send_len;
> - } else if (retry-- > 0) {
> - if (errno != ENOMEM &&
> - errno != ENOBUFS &&
> + } else if (retry >= 0) {
> + if (errno != EAGAIN && errno != EWOULDBLOCK &&
> + errno != ENOMEM && errno != ENOBUFS &&
> errno != EINTR)
> break;
> osaf_nanosleep();
[Minh] We may need to do error-log the strerror and errno in case of
failure in mds_retry_sendto(). Also,
[Thuan] error-log already done by upper callers.
uint32_t TipcPortId::Send(uint8_t* data, uint16_t length) {
...
m_MDS_LOG_ERR("FCTRL: sendto() failed, Error[%s]", strerror(errno));
}
this logging "sendto()" should be now "TipcPortId::Send()"
[Thuan] OK, will update this log message.
> @@ -3242,7 +3242,7 @@ static uint32_t mdtm_sendto(uint8_t *buffer, uint16_t
> buff_len,
> if (mds_tipc_fctrl_trysend(id, buffer, buff_len, is_queued)
> == NCSCC_RC_SUCCESS) {
> send_len = mds_retry_sendto(
> - tipc_cb.BSRsock, buffer, buff_len, 0,
> + tipc_cb.BSRsock, buffer, buff_len, MSG_DONTWAIT,
> (struct sockaddr *)_addr,
> sizeof(server_addr));
[Minh] There must be a reason that you want to use non-blocking with
MSG_DONTWAIT?
[Thuan] without that flag, ckpt 20 11 hang in sendto() then fctrl control
thread cannot handle anything.
> if (send_len == buff_len) {
> m_MDS_LOG_INFO("MDTM: Successfully sent message");
> @@ -3289,7 +3289,7 @@ static uint32_t mdtm_mcast_sendto(void *buffer, size_t
> size,
> /*This can be scope-down to dest_svc_id server_inst TBD*/
> server_addr.addr.nameseq.upper = HTONL(MDS_MDTM_UPPER_INSTANCE);
> ssize_t send_len =
> - mds_retry_sendto(tipc_cb.BSRsock, buffer, size, 0,
> + mds_retry_sendto(tipc_cb.BSRsock, buffer, size, MSG_DONTWAIT,
> (struct sockaddr *)_addr, sizeof(server_addr));
>
> if (send_len == size) {
> diff --git a/src/mds/mds_tipc_fctrl_intf.cc b/src/mds/mds_tipc_fctrl_intf.cc
> index 7d0571e7c..b20205686 100644
> --- a/src/mds/mds_tipc_fctrl_intf.cc
> +++ b/src/mds/mds_tipc_fctrl_intf.cc
> @@ -102,6 +102,8 @@ void tmr_exp_cbk(void* uarg) {
>
> void process_timer_event(const Event& evt) {
> bool txprob_restart = false;
> + m_MDS_LOG_DBG("FCTRL: process timer event start [evt:%d]",
> +static_cast(evt.type_));
> for (auto i : portid_map) {
> TipcPortId* portid = i.second;
>
> @@ -113,16 +115,20 @@ void process_timer_event(const Event& evt) {
>
> if (evt.type_ == Event::Type::kEvtTmrChunkAck) {
> portid->ReceiveTmrChunkAck();
> + portid->SendUnsentMsg();
> }
[Minh] The idea now is using ChunkAck timer to continue sending unsent
message. This fix comes from a situation that we failed in the middle of
sending unsent message due to "Cannot allocate memory...". In the
scenario without such error "Cannot allocate ...", the function
SendUnsentMsg() here will be sending extra messages from the "receiving
channel" as ChunkAck timer apart from
Re: [devel] [PATCH 1/1] mds: fix ckpt 20 11 failure [#3127]
Hi Thuan,
- We could give the patch title a bit more meanings than "fix ckpt 20
11..", for example, something as "Using timer to continue sending queued
message".
- And a few comments inline
Thanks
Minh
On 5/12/19 3:05 pm, thuan.tran wrote:
- In overflow, receive chunk ack may stuck in retrying to send pending
messages then later chunk ack comming cannot proceed.
- Instead of retrying to send pending messages, reuse timer send chunk
ack to trigger send pending messages if any. By this, even no more Nack
or ChunkAck event comming, pending messages will be sent by timer.
---
src/mds/mds_dt_tipc.c| 12 ++---
src/mds/mds_tipc_fctrl_intf.cc | 10
src/mds/mds_tipc_fctrl_portid.cc | 88 ++--
src/mds/mds_tipc_fctrl_portid.h | 1 +
4 files changed, 56 insertions(+), 55 deletions(-)
diff --git a/src/mds/mds_dt_tipc.c b/src/mds/mds_dt_tipc.c
index 9b3290833..6b30846a1 100644
--- a/src/mds/mds_dt_tipc.c
+++ b/src/mds/mds_dt_tipc.c
@@ -3183,13 +3183,13 @@ ssize_t mds_retry_sendto(int sockfd, const void *buf,
size_t len, int flags,
{
int retry = 5;
ssize_t send_len = 0;
- while (retry >= 0) {
+ while (retry-- >= 0) {
send_len = sendto(sockfd, buf, len, flags, dest_addr, addrlen);
if (send_len == len) {
return send_len;
- } else if (retry-- > 0) {
- if (errno != ENOMEM &&
- errno != ENOBUFS &&
+ } else if (retry >= 0) {
+ if (errno != EAGAIN && errno != EWOULDBLOCK &&
+ errno != ENOMEM && errno != ENOBUFS &&
errno != EINTR)
break;
osaf_nanosleep();
[Minh] We may need to do error-log the strerror and errno in case of
failure in mds_retry_sendto(). Also,
uint32_t TipcPortId::Send(uint8_t* data, uint16_t length) {
...
m_MDS_LOG_ERR("FCTRL: sendto() failed, Error[%s]", strerror(errno));
}
this logging "sendto()" should be now "TipcPortId::Send()"
@@ -3242,7 +3242,7 @@ static uint32_t mdtm_sendto(uint8_t *buffer, uint16_t
buff_len,
if (mds_tipc_fctrl_trysend(id, buffer, buff_len, is_queued)
== NCSCC_RC_SUCCESS) {
send_len = mds_retry_sendto(
- tipc_cb.BSRsock, buffer, buff_len, 0,
+ tipc_cb.BSRsock, buffer, buff_len, MSG_DONTWAIT,
(struct sockaddr *)_addr,
sizeof(server_addr));
[Minh] There must be a reason that you want to use non-blocking with
MSG_DONTWAIT?
if (send_len == buff_len) {
m_MDS_LOG_INFO("MDTM: Successfully sent message");
@@ -3289,7 +3289,7 @@ static uint32_t mdtm_mcast_sendto(void *buffer, size_t
size,
/*This can be scope-down to dest_svc_id server_inst TBD*/
server_addr.addr.nameseq.upper = HTONL(MDS_MDTM_UPPER_INSTANCE);
ssize_t send_len =
- mds_retry_sendto(tipc_cb.BSRsock, buffer, size, 0,
+ mds_retry_sendto(tipc_cb.BSRsock, buffer, size, MSG_DONTWAIT,
(struct sockaddr *)_addr, sizeof(server_addr));
if (send_len == size) {
diff --git a/src/mds/mds_tipc_fctrl_intf.cc b/src/mds/mds_tipc_fctrl_intf.cc
index 7d0571e7c..b20205686 100644
--- a/src/mds/mds_tipc_fctrl_intf.cc
+++ b/src/mds/mds_tipc_fctrl_intf.cc
@@ -102,6 +102,8 @@ void tmr_exp_cbk(void* uarg) {
void process_timer_event(const Event& evt) {
bool txprob_restart = false;
+ m_MDS_LOG_DBG("FCTRL: process timer event start [evt:%d]",
+static_cast(evt.type_));
for (auto i : portid_map) {
TipcPortId* portid = i.second;
@@ -113,16 +115,20 @@ void process_timer_event(const Event& evt) {
if (evt.type_ == Event::Type::kEvtTmrChunkAck) {
portid->ReceiveTmrChunkAck();
+ portid->SendUnsentMsg();
}
[Minh] The idea now is using ChunkAck timer to continue sending unsent
message. This fix comes from a situation that we failed in the middle of
sending unsent message due to "Cannot allocate memory...". In the
scenario without such error "Cannot allocate ...", the function
SendUnsentMsg() here will be sending extra messages from the "receiving
channel" as ChunkAck timer apart from the "sending channel" as
ReceiveChunkAck(). That would cause more undeliverable messages (the
ones are now sent from ChunkAck timer) if the overloading starts to
happen and sender keeps pushing more messages to send (more message
pushes into queue).
}
if (txprob_restart) {
txprob_timer.Start(kBaseTimerInt, tmr_exp_cbk);
m_MDS_LOG_DBG("FCTRL: Restart txprob");
}
+ m_MDS_LOG_DBG("FCTRL: process timer event end");
}
uint32_t process_flow_event(const Event& evt) {
uint32_t rc = NCSCC_RC_SUCCESS;
+ m_MDS_LOG_DBG("FCTRL: process flow event start [evt:%d]",
+static_cast(evt.type_));
Re: [devel] [PATCH 1/5] log: improve the resilience of log service [#3116]
Hi Vu, Thanks. See my replies inline. Best Regards, ThuanTr -Original Message- From: Nguyen Minh Vu Sent: Monday, December 9, 2019 6:18 PM To: Tran Thuan ; lennart.l...@ericsson.com; gary@dektech.com.au; minh.c...@dektech.com.au Cc: opensaf-devel@lists.sourceforge.net Subject: Re: [devel] [PATCH 1/5] log: improve the resilience of log service [#3116] Hi Thuan, See my responses inline. Regards, Vu On 12/9/19 5:32 PM, Tran Thuan wrote: > Hi Vu, > > Some comments from me: > > - I think need remove xid name in code. OK > - CleanOverdueData() should loop to clean all overdue records stead of just > one overdue record. No. It should only serve one element each time to avoid blocking the main thread. [Thuan] OK, the function name Clean/Flush make me think about MANY (ALL) Please consider to rename these functions. E.g: PopOverdueData() > - In PeriodicCheck, don't need check is_iothread_ready() before Flush() > because it is checked inside Flush() Ok. I will remove the check in `PeriodicCheck`. > - Flush() mean write all records, but actually just try to write one log > record, I think should rename it. Ok. Will rename it to 'FlushFrontElement`. [Thuan] Avoid Flush, maybe WriteFrontElement or PopDataToWrite() > > Best Regards, > ThuanTr > > -Original Message- > From: Vu Minh Nguyen > Sent: Thursday, November 28, 2019 3:24 PM > To: lennart.l...@ericsson.com; gary@dektech.com.au; > minh.c...@dektech.com.au > Cc: opensaf-devel@lists.sourceforge.net > Subject: [devel] [PATCH 1/5] log: improve the resilience of log service > [#3116] > > In order to improve resilience of OpenSAF LOG service when underlying > file system is unresponsive, a queue is introduced to hold async > write request up to an configurable time that is around 15 - 30 seconds. > > The readiness of the I/O thread will periodically check, and if it turns > to ready state, the front element will go first. Returns SA_AIS_ERR_TRY_AGAIN > to client if the element stays in the queue longer than the setting time. > > The queue capacity and the resilient time are configurable via the attributes: > `logMaxPendingWriteRequests` and `logResilienceTimeout`. > > In default, this feature is disabled to keep log server backward compatible. > --- > src/log/Makefile.am | 21 +- > src/log/config/logsv_classes.xml | 43 ++- > src/log/logd/lgs_cache.cc| 469 +++ > src/log/logd/lgs_cache.h | 287 +++ > src/log/logd/lgs_config.cc | 78 - > src/log/logd/lgs_config.h| 10 +- > src/log/logd/lgs_evt.cc | 161 +++ > src/log/logd/lgs_evt.h | 10 + > src/log/logd/lgs_file.cc | 8 +- > src/log/logd/lgs_filehdl.cc | 58 ++-- > src/log/logd/lgs_imm.cc | 40 ++- > src/log/logd/lgs_main.cc | 24 +- > src/log/logd/lgs_mbcsv.cc| 447 +++-- > src/log/logd/lgs_mbcsv.h | 19 +- > src/log/logd/lgs_mbcsv_cache.cc | 372 > src/log/logd/lgs_mbcsv_cache.h | 110 > src/log/logd/lgs_mbcsv_v1.cc | 1 + > src/log/logd/lgs_mbcsv_v2.cc | 2 + > 18 files changed, 1889 insertions(+), 271 deletions(-) > create mode 100644 src/log/logd/lgs_cache.cc > create mode 100644 src/log/logd/lgs_cache.h > create mode 100644 src/log/logd/lgs_mbcsv_cache.cc > create mode 100644 src/log/logd/lgs_mbcsv_cache.h > > diff --git a/src/log/Makefile.am b/src/log/Makefile.am > index f63a4a053..3367ef4f6 100644 > --- a/src/log/Makefile.am > +++ b/src/log/Makefile.am > @@ -95,7 +95,9 @@ noinst_HEADERS += \ > src/log/logd/lgs_nildest.h \ > src/log/logd/lgs_unixsock_dest.h \ > src/log/logd/lgs_common.h \ > - src/log/logd/lgs_amf.h > + src/log/logd/lgs_amf.h \ > + src/log/logd/lgs_cache.h \ > + src/log/logd/lgs_mbcsv_cache.h > > > bin_PROGRAMS += bin/saflogger > @@ -123,6 +125,15 @@ bin_osaflogd_CPPFLAGS = \ > -DSA_EXTENDED_NAME_SOURCE \ > $(AM_CPPFLAGS) > > +# Enable this flag to simulate the case that file system is unresponsive > +# during write log record. Mainly for testing the following enhancement: > +# log: improve the resilience of log service [#3116]. > +# When enabled, log handle thread will be suspended 17 seconds every 02 write > +# requests and only take affect if the `logMaxPendingWriteRequests` is set > +# to an non-zero value. > +bin_osaflogd_CPPFLAGS += -DSIMULATE_NFS_UNRESPONSE > + > + > bin_osaflogd_SOURCES = \ > src/log/logd/lgs_amf.cc \ > src/log/logd/lgs_clm.cc \ > @@ -147,7 +158,9 @@ bin_osaflogd_SOURCES = \ > src/log/logd/lgs_util.cc \ > src/log/logd/lgs_dest.cc \ > src/log/logd/lgs_nildest.cc \ > - src/log/logd/lgs_unixsock_dest.cc > + src/log/logd/lgs_unixsock_dest.cc \ > + src/log/logd/lgs_cache.cc \ > + src/log/logd/lgs_mbcsv_cache.cc > > bin_osaflogd_LDADD = \ >
Re: [devel] [PATCH 5/5] log: add test cases of improving the log resilience [#3116]
Hi Vu,
Thanks. See my reply inline.
Best Regards,
ThuanTr
-Original Message-
From: Nguyen Minh Vu
Sent: Monday, December 9, 2019 6:30 PM
To: Tran Thuan ; lennart.l...@ericsson.com;
gary@dektech.com.au; minh.c...@dektech.com.au
Cc: opensaf-devel@lists.sourceforge.net
Subject: Re: [devel] [PATCH 5/5] log: add test cases of improving the log
resilience [#3116]
Hi Thuan,
See my responses inline.
Regards, Vu
On 12/9/19 5:35 PM, Tran Thuan wrote:
> Hi Vu,
>
> Some comments from me:
>
> - Remove xid in test code.
OK
> - Use AIS_EVALUATE() and SYS_EVALUATE() that you defined, but you still
> direct use test_validate()/rc_validate()
The macros are used to validate the given pre-condition. If the
condition does not meet, the test will get failed
and discontinue. These are used for different purposes.
[Thuan] OK, I see. But I don't see anywhere use SYS_EVALUATE()
> - polling_thread() is create thread and wait it done, why not directly call
> the function?
OK. Good point.
> - In case of reboot SCs for headless, can we use system() command with
> following? Then no need manual interaction.
> ssh SC-1 "rdegetrole |grep ACTIVE && reboot"
> ssh SC-1 "rdegetrole |grep STANDBY && reboot"
ssh does not work on UML containers.
[Thuan] Can we use "immadm -o 5 -p saClmAction:SA_STRING_T:stop "?
Just hope to make all test can run without manual interaction.
>
>
> Best Regards,
> ThuanTr
>
> -Original Message-
> From: Vu Minh Nguyen
> Sent: Thursday, November 28, 2019 3:25 PM
> To: lennart.l...@ericsson.com; gary@dektech.com.au;
> minh.c...@dektech.com.au
> Cc: opensaf-devel@lists.sourceforge.net
> Subject: [devel] [PATCH 5/5] log: add test cases of improving the log
> resilience [#3116]
>
> Adding 08 new test cases into 02 suites:
> 1) Suite 20 with 07 test cases, including:
> - Test changing queue size & resilient timeout;
> - Test if a write async is dropped if its timeout setting is overdue,
> also verify if log server has kept the request in proper time.
> - Test if getting write callback right away if the cache is full.
> - Test if the cache is fully and correctly synced with standby.
>
> 2) Suite 21 with one test case:
> Test if LOG agent notifies all lost invocation to log client.
>
> As the suite 21 requires manual interaction, it is put into
> 'extended' tests. Only run with option '-e'.
> ---
> src/log/Makefile.am | 3 +-
> src/log/apitest/logtest.c | 7 +
> src/log/apitest/logtest.h | 7 +-
> src/log/apitest/logutil.c | 14 +-
> src/log/apitest/tet_log_runtime_cfgobj.c | 2 +-
> .../apitest/tet_saLogWriteLogAsync_cache.c| 648 ++
> 6 files changed, 667 insertions(+), 14 deletions(-)
> create mode 100644 src/log/apitest/tet_saLogWriteLogAsync_cache.c
>
> diff --git a/src/log/Makefile.am b/src/log/Makefile.am
> index 3367ef4f6..3ec03c097 100644
> --- a/src/log/Makefile.am
> +++ b/src/log/Makefile.am
> @@ -224,7 +224,8 @@ bin_logtest_SOURCES = \
> src/log/apitest/tet_log_longDN.c \
> src/log/apitest/tet_Log_clm.c \
> src/log/apitest/tet_cfg_destination.c \
> - src/log/apitest/tet_multiple_thread.c
> + src/log/apitest/tet_multiple_thread.c \
> + src/log/apitest/tet_saLogWriteLogAsync_cache.c
>
> bin_logtest_LDADD = \
> lib/libapitest.la \
> diff --git a/src/log/apitest/logtest.c b/src/log/apitest/logtest.c
> index aabd1e578..149d27d93 100644
> --- a/src/log/apitest/logtest.c
> +++ b/src/log/apitest/logtest.c
> @@ -96,6 +96,7 @@ SaLogCallbacksT logCallbacks = {NULL, NULL, NULL};
> SaInvocationT invocation = 0;
> SaSelectionObjectT selectionObject;
> char log_root_path[PATH_MAX];
> +SaLogAckFlagsT ack_flags = 0;
>
> void init_logrootpath(void)
> {
> @@ -465,6 +466,9 @@ int main(int argc, char **argv)
> add_suite_14();
> add_suite_15();
> add_suite_16();
> +#ifdef SIMULATE_NFS_UNRESPONSE
> + add_suite_21();
> +#endif
> test_list();
> exit(0);
> case 'e':
> @@ -493,6 +497,9 @@ int main(int argc, char **argv)
> add_suite_14();
> add_suite_15();
> add_suite_16();
> +#ifdef SIMULATE_NFS_UNRESPONSE
> + add_suite_21();
> +#endif
> break;
> case 'v':
> if (silent_flg == true) {
> diff --git a/src/log/apitest/logtest.h b/src/log/apitest/logtest.h
> index 68f9df608..e04492086 100644
> --- a/src/log/apitest/logtest.h
> +++ b/src/log/apitest/logtest.h
> @@ -76,7 +76,7 @@ extern SaSelectionObjectT selectionObject;
> extern SaNameT logSvcUsrName;
> extern SaLogRecordT genLogRecord;
> extern char log_root_path[];
> -
> +extern SaLogAckFlagsT ack_flags;
> const static SaVersionT kLogVersion = {'A', 0x02, 0x03};
>
Re: [devel] [PATCH 5/5] log: add test cases of improving the log resilience [#3116]
Hi Thuan,
See my responses inline.
Regards, Vu
On 12/9/19 5:35 PM, Tran Thuan wrote:
Hi Vu,
Some comments from me:
- Remove xid in test code.
OK
- Use AIS_EVALUATE() and SYS_EVALUATE() that you defined, but you still direct
use test_validate()/rc_validate()
The macros are used to validate the given pre-condition. If the
condition does not meet, the test will get failed
and discontinue. These are used for different purposes.
- polling_thread() is create thread and wait it done, why not directly call the
function?
OK. Good point.
- In case of reboot SCs for headless, can we use system() command with
following? Then no need manual interaction.
ssh SC-1 "rdegetrole |grep ACTIVE && reboot"
ssh SC-1 "rdegetrole |grep STANDBY && reboot"
ssh does not work on UML containers.
Best Regards,
ThuanTr
-Original Message-
From: Vu Minh Nguyen
Sent: Thursday, November 28, 2019 3:25 PM
To: lennart.l...@ericsson.com; gary@dektech.com.au; minh.c...@dektech.com.au
Cc: opensaf-devel@lists.sourceforge.net
Subject: [devel] [PATCH 5/5] log: add test cases of improving the log
resilience [#3116]
Adding 08 new test cases into 02 suites:
1) Suite 20 with 07 test cases, including:
- Test changing queue size & resilient timeout;
- Test if a write async is dropped if its timeout setting is overdue,
also verify if log server has kept the request in proper time.
- Test if getting write callback right away if the cache is full.
- Test if the cache is fully and correctly synced with standby.
2) Suite 21 with one test case:
Test if LOG agent notifies all lost invocation to log client.
As the suite 21 requires manual interaction, it is put into
'extended' tests. Only run with option '-e'.
---
src/log/Makefile.am | 3 +-
src/log/apitest/logtest.c | 7 +
src/log/apitest/logtest.h | 7 +-
src/log/apitest/logutil.c | 14 +-
src/log/apitest/tet_log_runtime_cfgobj.c | 2 +-
.../apitest/tet_saLogWriteLogAsync_cache.c| 648 ++
6 files changed, 667 insertions(+), 14 deletions(-)
create mode 100644 src/log/apitest/tet_saLogWriteLogAsync_cache.c
diff --git a/src/log/Makefile.am b/src/log/Makefile.am
index 3367ef4f6..3ec03c097 100644
--- a/src/log/Makefile.am
+++ b/src/log/Makefile.am
@@ -224,7 +224,8 @@ bin_logtest_SOURCES = \
src/log/apitest/tet_log_longDN.c \
src/log/apitest/tet_Log_clm.c \
src/log/apitest/tet_cfg_destination.c \
- src/log/apitest/tet_multiple_thread.c
+ src/log/apitest/tet_multiple_thread.c \
+ src/log/apitest/tet_saLogWriteLogAsync_cache.c
bin_logtest_LDADD = \
lib/libapitest.la \
diff --git a/src/log/apitest/logtest.c b/src/log/apitest/logtest.c
index aabd1e578..149d27d93 100644
--- a/src/log/apitest/logtest.c
+++ b/src/log/apitest/logtest.c
@@ -96,6 +96,7 @@ SaLogCallbacksT logCallbacks = {NULL, NULL, NULL};
SaInvocationT invocation = 0;
SaSelectionObjectT selectionObject;
char log_root_path[PATH_MAX];
+SaLogAckFlagsT ack_flags = 0;
void init_logrootpath(void)
{
@@ -465,6 +466,9 @@ int main(int argc, char **argv)
add_suite_14();
add_suite_15();
add_suite_16();
+#ifdef SIMULATE_NFS_UNRESPONSE
+ add_suite_21();
+#endif
test_list();
exit(0);
case 'e':
@@ -493,6 +497,9 @@ int main(int argc, char **argv)
add_suite_14();
add_suite_15();
add_suite_16();
+#ifdef SIMULATE_NFS_UNRESPONSE
+ add_suite_21();
+#endif
break;
case 'v':
if (silent_flg == true) {
diff --git a/src/log/apitest/logtest.h b/src/log/apitest/logtest.h
index 68f9df608..e04492086 100644
--- a/src/log/apitest/logtest.h
+++ b/src/log/apitest/logtest.h
@@ -76,7 +76,7 @@ extern SaSelectionObjectT selectionObject;
extern SaNameT logSvcUsrName;
extern SaLogRecordT genLogRecord;
extern char log_root_path[];
-
+extern SaLogAckFlagsT ack_flags;
const static SaVersionT kLogVersion = {'A', 0x02, 0x03};
const static SaVersionT kImmVersion = {'A', 02, 11};
@@ -105,6 +105,11 @@ void add_suite_12(void);
void add_suite_14();
void add_suite_15();
void add_suite_16();
+
+#ifdef SIMULATE_NFS_UNRESPONSE
+void add_suite_21();
+#endif
+
int get_active_sc(void);
int get_attr_value(SaNameT *inObjName, char *inAttr, void *outValue);
diff --git a/src/log/apitest/logutil.c b/src/log/apitest/logutil.c
index 59d255515..d3e0c6297 100644
--- a/src/log/apitest/logutil.c
+++ b/src/log/apitest/logutil.c
@@ -52,15 +52,7 @@ void cond_check(void)
int systemCall(const char *command)
{
int rc = system(command);
- if (rc == -1) {
- fprintf(stderr, "system() retuned -1 Failed \n");
- } else {
-
Re: [devel] [PATCH 1/5] log: improve the resilience of log service [#3116]
Hi Thuan, See my responses inline. Regards, Vu On 12/9/19 5:32 PM, Tran Thuan wrote: Hi Vu, Some comments from me: - I think need remove xid name in code. OK - CleanOverdueData() should loop to clean all overdue records stead of just one overdue record. No. It should only serve one element each time to avoid blocking the main thread. - In PeriodicCheck, don't need check is_iothread_ready() before Flush() because it is checked inside Flush() Ok. I will remove the check in `PeriodicCheck`. - Flush() mean write all records, but actually just try to write one log record, I think should rename it. Ok. Will rename it to 'FlushFrontElement`. Best Regards, ThuanTr -Original Message- From: Vu Minh Nguyen Sent: Thursday, November 28, 2019 3:24 PM To: lennart.l...@ericsson.com; gary@dektech.com.au; minh.c...@dektech.com.au Cc: opensaf-devel@lists.sourceforge.net Subject: [devel] [PATCH 1/5] log: improve the resilience of log service [#3116] In order to improve resilience of OpenSAF LOG service when underlying file system is unresponsive, a queue is introduced to hold async write request up to an configurable time that is around 15 - 30 seconds. The readiness of the I/O thread will periodically check, and if it turns to ready state, the front element will go first. Returns SA_AIS_ERR_TRY_AGAIN to client if the element stays in the queue longer than the setting time. The queue capacity and the resilient time are configurable via the attributes: `logMaxPendingWriteRequests` and `logResilienceTimeout`. In default, this feature is disabled to keep log server backward compatible. --- src/log/Makefile.am | 21 +- src/log/config/logsv_classes.xml | 43 ++- src/log/logd/lgs_cache.cc| 469 +++ src/log/logd/lgs_cache.h | 287 +++ src/log/logd/lgs_config.cc | 78 - src/log/logd/lgs_config.h| 10 +- src/log/logd/lgs_evt.cc | 161 +++ src/log/logd/lgs_evt.h | 10 + src/log/logd/lgs_file.cc | 8 +- src/log/logd/lgs_filehdl.cc | 58 ++-- src/log/logd/lgs_imm.cc | 40 ++- src/log/logd/lgs_main.cc | 24 +- src/log/logd/lgs_mbcsv.cc| 447 +++-- src/log/logd/lgs_mbcsv.h | 19 +- src/log/logd/lgs_mbcsv_cache.cc | 372 src/log/logd/lgs_mbcsv_cache.h | 110 src/log/logd/lgs_mbcsv_v1.cc | 1 + src/log/logd/lgs_mbcsv_v2.cc | 2 + 18 files changed, 1889 insertions(+), 271 deletions(-) create mode 100644 src/log/logd/lgs_cache.cc create mode 100644 src/log/logd/lgs_cache.h create mode 100644 src/log/logd/lgs_mbcsv_cache.cc create mode 100644 src/log/logd/lgs_mbcsv_cache.h diff --git a/src/log/Makefile.am b/src/log/Makefile.am index f63a4a053..3367ef4f6 100644 --- a/src/log/Makefile.am +++ b/src/log/Makefile.am @@ -95,7 +95,9 @@ noinst_HEADERS += \ src/log/logd/lgs_nildest.h \ src/log/logd/lgs_unixsock_dest.h \ src/log/logd/lgs_common.h \ - src/log/logd/lgs_amf.h + src/log/logd/lgs_amf.h \ + src/log/logd/lgs_cache.h \ + src/log/logd/lgs_mbcsv_cache.h bin_PROGRAMS += bin/saflogger @@ -123,6 +125,15 @@ bin_osaflogd_CPPFLAGS = \ -DSA_EXTENDED_NAME_SOURCE \ $(AM_CPPFLAGS) +# Enable this flag to simulate the case that file system is unresponsive +# during write log record. Mainly for testing the following enhancement: +# log: improve the resilience of log service [#3116]. +# When enabled, log handle thread will be suspended 17 seconds every 02 write +# requests and only take affect if the `logMaxPendingWriteRequests` is set +# to an non-zero value. +bin_osaflogd_CPPFLAGS += -DSIMULATE_NFS_UNRESPONSE + + bin_osaflogd_SOURCES = \ src/log/logd/lgs_amf.cc \ src/log/logd/lgs_clm.cc \ @@ -147,7 +158,9 @@ bin_osaflogd_SOURCES = \ src/log/logd/lgs_util.cc \ src/log/logd/lgs_dest.cc \ src/log/logd/lgs_nildest.cc \ - src/log/logd/lgs_unixsock_dest.cc + src/log/logd/lgs_unixsock_dest.cc \ + src/log/logd/lgs_cache.cc \ + src/log/logd/lgs_mbcsv_cache.cc bin_osaflogd_LDADD = \ lib/libosaf_common.la \ @@ -183,6 +196,10 @@ bin_logtest_CPPFLAGS = \ -DSA_EXTENDED_NAME_SOURCE \ $(AM_CPPFLAGS) +# Enable this flag to add test cases for following enhancement: +# log: improve the resilience of log service [#3116]. +bin_logtest_CPPFLAGS += -DSIMULATE_NFS_UNRESPONSE + bin_logtest_SOURCES = \ src/log/apitest/logtest.c \ src/log/apitest/logutil.c \ diff --git a/src/log/config/logsv_classes.xml b/src/log/config/logsv_classes.xml index 9359823ff..084e8915d 100644 --- a/src/log/config/logsv_classes.xml +++ b/src/log/config/logsv_classes.xml @@ -195,7 +195,7 @@ to ensure that default global values in the implementation are also changed acco SA_UINT32_T
Re: [devel] [PATCH 5/5] log: add test cases of improving the log resilience [#3116]
Hi Vu,
Some comments from me:
- Remove xid in test code.
- Use AIS_EVALUATE() and SYS_EVALUATE() that you defined, but you still direct
use test_validate()/rc_validate()
- polling_thread() is create thread and wait it done, why not directly call the
function?
- In case of reboot SCs for headless, can we use system() command with
following? Then no need manual interaction.
ssh SC-1 "rdegetrole |grep ACTIVE && reboot"
ssh SC-1 "rdegetrole |grep STANDBY && reboot"
Best Regards,
ThuanTr
-Original Message-
From: Vu Minh Nguyen
Sent: Thursday, November 28, 2019 3:25 PM
To: lennart.l...@ericsson.com; gary@dektech.com.au; minh.c...@dektech.com.au
Cc: opensaf-devel@lists.sourceforge.net
Subject: [devel] [PATCH 5/5] log: add test cases of improving the log
resilience [#3116]
Adding 08 new test cases into 02 suites:
1) Suite 20 with 07 test cases, including:
- Test changing queue size & resilient timeout;
- Test if a write async is dropped if its timeout setting is overdue,
also verify if log server has kept the request in proper time.
- Test if getting write callback right away if the cache is full.
- Test if the cache is fully and correctly synced with standby.
2) Suite 21 with one test case:
Test if LOG agent notifies all lost invocation to log client.
As the suite 21 requires manual interaction, it is put into
'extended' tests. Only run with option '-e'.
---
src/log/Makefile.am | 3 +-
src/log/apitest/logtest.c | 7 +
src/log/apitest/logtest.h | 7 +-
src/log/apitest/logutil.c | 14 +-
src/log/apitest/tet_log_runtime_cfgobj.c | 2 +-
.../apitest/tet_saLogWriteLogAsync_cache.c| 648 ++
6 files changed, 667 insertions(+), 14 deletions(-)
create mode 100644 src/log/apitest/tet_saLogWriteLogAsync_cache.c
diff --git a/src/log/Makefile.am b/src/log/Makefile.am
index 3367ef4f6..3ec03c097 100644
--- a/src/log/Makefile.am
+++ b/src/log/Makefile.am
@@ -224,7 +224,8 @@ bin_logtest_SOURCES = \
src/log/apitest/tet_log_longDN.c \
src/log/apitest/tet_Log_clm.c \
src/log/apitest/tet_cfg_destination.c \
- src/log/apitest/tet_multiple_thread.c
+ src/log/apitest/tet_multiple_thread.c \
+ src/log/apitest/tet_saLogWriteLogAsync_cache.c
bin_logtest_LDADD = \
lib/libapitest.la \
diff --git a/src/log/apitest/logtest.c b/src/log/apitest/logtest.c
index aabd1e578..149d27d93 100644
--- a/src/log/apitest/logtest.c
+++ b/src/log/apitest/logtest.c
@@ -96,6 +96,7 @@ SaLogCallbacksT logCallbacks = {NULL, NULL, NULL};
SaInvocationT invocation = 0;
SaSelectionObjectT selectionObject;
char log_root_path[PATH_MAX];
+SaLogAckFlagsT ack_flags = 0;
void init_logrootpath(void)
{
@@ -465,6 +466,9 @@ int main(int argc, char **argv)
add_suite_14();
add_suite_15();
add_suite_16();
+#ifdef SIMULATE_NFS_UNRESPONSE
+ add_suite_21();
+#endif
test_list();
exit(0);
case 'e':
@@ -493,6 +497,9 @@ int main(int argc, char **argv)
add_suite_14();
add_suite_15();
add_suite_16();
+#ifdef SIMULATE_NFS_UNRESPONSE
+ add_suite_21();
+#endif
break;
case 'v':
if (silent_flg == true) {
diff --git a/src/log/apitest/logtest.h b/src/log/apitest/logtest.h
index 68f9df608..e04492086 100644
--- a/src/log/apitest/logtest.h
+++ b/src/log/apitest/logtest.h
@@ -76,7 +76,7 @@ extern SaSelectionObjectT selectionObject;
extern SaNameT logSvcUsrName;
extern SaLogRecordT genLogRecord;
extern char log_root_path[];
-
+extern SaLogAckFlagsT ack_flags;
const static SaVersionT kLogVersion = {'A', 0x02, 0x03};
const static SaVersionT kImmVersion = {'A', 02, 11};
@@ -105,6 +105,11 @@ void add_suite_12(void);
void add_suite_14();
void add_suite_15();
void add_suite_16();
+
+#ifdef SIMULATE_NFS_UNRESPONSE
+void add_suite_21();
+#endif
+
int get_active_sc(void);
int get_attr_value(SaNameT *inObjName, char *inAttr, void *outValue);
diff --git a/src/log/apitest/logutil.c b/src/log/apitest/logutil.c
index 59d255515..d3e0c6297 100644
--- a/src/log/apitest/logutil.c
+++ b/src/log/apitest/logutil.c
@@ -52,15 +52,7 @@ void cond_check(void)
int systemCall(const char *command)
{
int rc = system(command);
- if (rc == -1) {
- fprintf(stderr, "system() retuned -1 Failed \n");
- } else {
- rc = WEXITSTATUS(rc);
- if (rc != 0)
- fprintf(stderr, " Failed in command: %s \n", command);
- }
-
- return rc;
+ return WEXITSTATUS(rc);
}
/*
@@ -144,8 +136,8 @@ logAppStreamOpen(const SaNameT *logStreamName,
*/
SaAisErrorT logWriteAsync(const SaLogRecordT *logRecord)
{
-
Re: [devel] [PATCH 1/5] log: improve the resilience of log service [#3116]
Hi Vu, Some comments from me: - I think need remove xid name in code. - CleanOverdueData() should loop to clean all overdue records stead of just one overdue record. - In PeriodicCheck, don't need check is_iothread_ready() before Flush() because it is checked inside Flush() - Flush() mean write all records, but actually just try to write one log record, I think should rename it. Best Regards, ThuanTr -Original Message- From: Vu Minh Nguyen Sent: Thursday, November 28, 2019 3:24 PM To: lennart.l...@ericsson.com; gary@dektech.com.au; minh.c...@dektech.com.au Cc: opensaf-devel@lists.sourceforge.net Subject: [devel] [PATCH 1/5] log: improve the resilience of log service [#3116] In order to improve resilience of OpenSAF LOG service when underlying file system is unresponsive, a queue is introduced to hold async write request up to an configurable time that is around 15 - 30 seconds. The readiness of the I/O thread will periodically check, and if it turns to ready state, the front element will go first. Returns SA_AIS_ERR_TRY_AGAIN to client if the element stays in the queue longer than the setting time. The queue capacity and the resilient time are configurable via the attributes: `logMaxPendingWriteRequests` and `logResilienceTimeout`. In default, this feature is disabled to keep log server backward compatible. --- src/log/Makefile.am | 21 +- src/log/config/logsv_classes.xml | 43 ++- src/log/logd/lgs_cache.cc| 469 +++ src/log/logd/lgs_cache.h | 287 +++ src/log/logd/lgs_config.cc | 78 - src/log/logd/lgs_config.h| 10 +- src/log/logd/lgs_evt.cc | 161 +++ src/log/logd/lgs_evt.h | 10 + src/log/logd/lgs_file.cc | 8 +- src/log/logd/lgs_filehdl.cc | 58 ++-- src/log/logd/lgs_imm.cc | 40 ++- src/log/logd/lgs_main.cc | 24 +- src/log/logd/lgs_mbcsv.cc| 447 +++-- src/log/logd/lgs_mbcsv.h | 19 +- src/log/logd/lgs_mbcsv_cache.cc | 372 src/log/logd/lgs_mbcsv_cache.h | 110 src/log/logd/lgs_mbcsv_v1.cc | 1 + src/log/logd/lgs_mbcsv_v2.cc | 2 + 18 files changed, 1889 insertions(+), 271 deletions(-) create mode 100644 src/log/logd/lgs_cache.cc create mode 100644 src/log/logd/lgs_cache.h create mode 100644 src/log/logd/lgs_mbcsv_cache.cc create mode 100644 src/log/logd/lgs_mbcsv_cache.h diff --git a/src/log/Makefile.am b/src/log/Makefile.am index f63a4a053..3367ef4f6 100644 --- a/src/log/Makefile.am +++ b/src/log/Makefile.am @@ -95,7 +95,9 @@ noinst_HEADERS += \ src/log/logd/lgs_nildest.h \ src/log/logd/lgs_unixsock_dest.h \ src/log/logd/lgs_common.h \ - src/log/logd/lgs_amf.h + src/log/logd/lgs_amf.h \ + src/log/logd/lgs_cache.h \ + src/log/logd/lgs_mbcsv_cache.h bin_PROGRAMS += bin/saflogger @@ -123,6 +125,15 @@ bin_osaflogd_CPPFLAGS = \ -DSA_EXTENDED_NAME_SOURCE \ $(AM_CPPFLAGS) +# Enable this flag to simulate the case that file system is unresponsive +# during write log record. Mainly for testing the following enhancement: +# log: improve the resilience of log service [#3116]. +# When enabled, log handle thread will be suspended 17 seconds every 02 write +# requests and only take affect if the `logMaxPendingWriteRequests` is set +# to an non-zero value. +bin_osaflogd_CPPFLAGS += -DSIMULATE_NFS_UNRESPONSE + + bin_osaflogd_SOURCES = \ src/log/logd/lgs_amf.cc \ src/log/logd/lgs_clm.cc \ @@ -147,7 +158,9 @@ bin_osaflogd_SOURCES = \ src/log/logd/lgs_util.cc \ src/log/logd/lgs_dest.cc \ src/log/logd/lgs_nildest.cc \ - src/log/logd/lgs_unixsock_dest.cc + src/log/logd/lgs_unixsock_dest.cc \ + src/log/logd/lgs_cache.cc \ + src/log/logd/lgs_mbcsv_cache.cc bin_osaflogd_LDADD = \ lib/libosaf_common.la \ @@ -183,6 +196,10 @@ bin_logtest_CPPFLAGS = \ -DSA_EXTENDED_NAME_SOURCE \ $(AM_CPPFLAGS) +# Enable this flag to add test cases for following enhancement: +# log: improve the resilience of log service [#3116]. +bin_logtest_CPPFLAGS += -DSIMULATE_NFS_UNRESPONSE + bin_logtest_SOURCES = \ src/log/apitest/logtest.c \ src/log/apitest/logutil.c \ diff --git a/src/log/config/logsv_classes.xml b/src/log/config/logsv_classes.xml index 9359823ff..084e8915d 100644 --- a/src/log/config/logsv_classes.xml +++ b/src/log/config/logsv_classes.xml @@ -195,7 +195,7 @@ to ensure that default global values in the implementation are also changed acco SA_UINT32_T SA_CONFIG SA_WRITABLE -1024 + 1024 logStreamFileFormat @@ -208,42 +208,42 @@ to ensure that default global values in the implementation are also changed acco
